Faster "obj is T[]" for sealed T #75816
Conversation
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsLet's see if my assumptions were correct. Closes #75815 bool IsArrayOfStrings(object o) => o is string[]; // or e.g. IEnumerable<string> is string[]codegen:
|
|
🚀Finally! |
|
Relaxed checks made NativeAOT unhappy, let me see why |
Perhaps this optimization is not valid for NativeAOT for MD arrays? |
|
The optimization is valid for NativeAOT. The problem seems to be that the codegen is violating the JIT helper contract. NativeAOT asserts. CoreCLR has a test hole (note that this is nativeaot smoke test that does not run on CoreCLR), or it happens to work and not crash, but it does not make it right. |
hm.. I don't see an API to find out whether a type is |
I am not sure why you would need this. I think you just need to overwriting the helper to runtime/src/coreclr/jit/importer.cpp Line 12535 in 4cd3e78 |
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
…e-1 into faster-isinst-array-sealed
|
@jkotas thanks for suggestion, the code is cleaner now! @dotnet/jit-contrib PTAL |
src/coreclr/jit/importer.cpp
Outdated
| // | ||
| const CorInfoHelpFunc specialHelper = CORINFO_HELP_CHKCASTCLASS_SPECIAL; | ||
| const CorInfoHelpFunc specialHelper = | ||
| (helper == CORINFO_HELP_CHKCASTCLASS) ? CORINFO_HELP_CHKCASTCLASS_SPECIAL : helper; |
There was a problem hiding this comment.
What other helper we expect for isCastClass == true?
There was a problem hiding this comment.
@kunalspathak only CORINFO_HELP_CHKCASTCLASS that is replaced with CORINFO_HELP_CHKCASTCLASS_SPECIAL
other helpers ( CORINFO_HELP_CHKCASTARRAY and CORINFO_HELP_CHKCASTINTERFACE are expected to be untouched)
There was a problem hiding this comment.
Can we assert(helper == CORINFO_HELP_CHKCASTARRAY)?
There was a problem hiding this comment.
CORINFO_HELP_CHKCASTINTERFACE wasn't handled previously (sounds like a bug) but it was only ever used with DOTNET_JitProfileCasts=1 which is 0 by default
There was a problem hiding this comment.
@jkotas actually it looks like the check you asked me to delete is still needed, consider this example:
class ClassA { }
class ClassB : ClassA { }
internal class Program
{
[MethodImpl(MethodImplOptions.NoInlining)]
public static void Main()
{
for (int i = 0; i < 100; i++)
{
CastToClassA(new ClassB());
Thread.Sleep(10);
}
}
[MethodImpl(MethodImplOptions.NoInlining)]
static ClassA CastToClassA(object o) => (ClassA)o;
}Current Tier1 codegen for CastToClassA is:
; Assembly listing for method CastToClassA
G_M33135_IG01:
4883EC28 sub rsp, 40
G_M33135_IG02:
48894C2430 mov gword ptr [rsp+30H], rcx
488BC1 mov rax, rcx
4885C0 test rax, rax
741D je SHORT G_M33135_IG05
G_M33135_IG03:
48BA18537F78FE7F0000 mov rdx, 0x7FFE787F5318 ; ClassA
483910 cmp qword ptr [rax], rdx
740E je SHORT G_M33135_IG05
G_M33135_IG04:
488BCA mov rcx, rdx
488B542430 mov rdx, gword ptr [rsp+30H]
FF152A75FBFF call [CORINFO_HELP_CHKCASTCLASS_SPECIAL]
G_M33135_IG05:
90 nop
G_M33135_IG06:
4883C428 add rsp, 40
C3 retSo jit inlined check for ClassA so it doesn't need to check for it inside the _SPECIAL helper. But as you can see from the call-site the check for ClassA fast path is not valid for given app in terms of performance. So with Dynamic PGO and DOTNET_JitProfileCasts=1 I get this:
; Assembly listing for method CastToClassA
G_M33135_IG01:
4883EC28 sub rsp, 40
G_M33135_IG02:
488BC1 mov rax, rcx
4885C0 test rax, rax
7422 je SHORT G_M33135_IG05
G_M33135_IG03:
48BA484F8078FE7F0000 mov rdx, 0x7FFE78804F48 ; ClassB
483910 cmp qword ptr [rax], rdx
7413 je SHORT G_M33135_IG05
G_M33135_IG04:
488BD1 mov rdx, rcx
48B9E04D8078FE7F0000 mov rcx, 0x7FFE78804DE0 ; ClassA
FF157A75FBFF call [CORINFO_HELP_CHKCASTCLASS_SPECIAL]
G_M33135_IG05:
90 nop
G_M33135_IG06:
4883C428 add rsp, 40
C3 retHere we check for ClassB for fast path but we'll need the check for itself in the helper. Here is the codegen diff between two (PGO is on the right):
There was a problem hiding this comment.
As an optimization, we can introduce CORINFO_HELP_CHKCASTCLASS_SPECIAL_SLOW with the "am I the same type?" check
There was a problem hiding this comment.
Alternatively, we can check for both in JIT inlined - but it doesn't sound right to me, if we have a profile saying that it's always ClassB why would we want to inline a check for ClassA?
There was a problem hiding this comment.
The JIT should be using the regular CORINFO_HELP_CHKCASTCLASS when it decides to inline a check for a type from the middle of the type hierarchy.
CORINFO_HELP_CHKCASTCLASS_SPECIAL_SLOW with the "am I the same type?" check
It is exactly what CORINFO_HELP_CHKCASTCLASS does. We do not need a new helper.
…e-1 into faster-isinst-array-sealed
src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs
Show resolved
Hide resolved
| GenTree* condFalse = gtClone(op1); | ||
| GenTree* condTrue; | ||
| if (isCastClass) | ||
| { |
There was a problem hiding this comment.
Is there a reason why we are not doing the PGO profiling for the CastAny helpers?
It would be nice to note that reason in impIsCastHelperMayHaveProfileData/impIsCastHelperMayHaveProfileData.
There was a problem hiding this comment.
I can look on enabling those separately if you don't mind - probably we can benefit from them a lot, will experiment. I assume they will cover a lot of generic code
There was a problem hiding this comment.
Yes, that's fine. It is just something that I have run into while reviewing this change.
|
Hm something is off on CI - with |
|
Improvements on Arm64: dotnet/perf-autofiling-issues#8663 |
ghost
locked as resolved and limited conversation to collaborators
5 participants
Let's see if my assumptions were correct. Closes #75815
codegen diff: https://www.diffchecker.com/IoRePQ7d
jit-diffs (409 methods affected)
Mostly size regression coming from
castclassexpansion which is expected.