Weird inlining of methods optimized by JIT

[Tearth]

Description

I've spotted some weird behavior in code generated by JIT. The image below is self-explaining but a short description from me: we have two similar methods, optimized to the same form (which is just a simple asm instruction, like blsr here). In theory, both should be inlined just looking at their size, but apparently, JIT does it only for the first method as shown below.

Data

Here is the link to this snippet: https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABABgAJiBGAOgCUBXAOwwEt8YaBhCfAB1YAbGFADKIgG6swMXAG4AsAChlTbB1x9sM8gAUIfADK5gAFVkZlAb2Xk7lAEyUqAdlv2bS+9+dJyDQQgmAHNyBEUvHztqP2IUcgBZbFYmAAoASncozyiohHIAXj0DY2AqVKpSUnSI3J98ov0jEwcKqpqsnwBfTu9e+xj/QJDi5rLUgKDQiWxBBhhMyOz+qOIXZ3pmNg4aAEkWKBTcaVwaAA0ADiQaACF8Vloz1HpZGAxDCAB3C3EMG9YMKkZnMFrVcj0lj4Vr4hlNRqVWpMRsD5os6jk6vZWAAzcipaibFjsTj7DCHJjHMCnS7XO4PPa4UQMPh8aAYGAAEzRmLsGJ53jWG0YRJ2pPJlOpV1u90ezzor3eXx+b3+gJRoOh3gh/PIMEEuBgmo8RtW63V5AAZHjzXByFQOpC6trwZ0IV0gA==

Analysis

We can force the second method to be inlined by [MethodImpl(MethodImplOptions.AggressiveInlining)], in this case, it behaves exactly the same as the first one.

category:cq
theme:inlining
skill-level:expert
cost:large

[tannergooding]

The JIT can't see that the calls in PopLsb2 are to intrinsics and so when deciding whether or not inlining is profitable, it just sees two calls and a computation, not that one of the code paths will be completely dead and therefore optimized away.

CC. @AndyAyersMS

[SingleAccretion]

To add some more to Tanner's explanation, here's the Jit's reasoning about the function (note that I used PopCount here as my CPU doesn't support BMI, but they are more or less identical from the inliner's perspective):

INLINER impTokenLookupContextHandle for JitBugReproduction.Program:PopcountWithCheck(long):long is 0x00000000D1FFAB1E.
*************** In fgFindBasicBlocks() for JitBugReproduction.Program:PopcountWithCheck(long):long
weight= 79 : state  40 [ call ]
weight= 27 : state  44 [ brfalse.s ]
weight= 10 : state   3 [ ldarg.0 ]
weight= 79 : state  40 [ call ]
weight= 19 : state  42 [ ret ]
weight= 10 : state   3 [ ldarg.0 ]
weight= 79 : state  40 [ call ]
weight= 99 : state  94 [ conv.i8 ]
weight= 19 : state  42 [ ret ]

Inline candidate looks like a wrapper method.  Multiplier increased to 1.
Inline candidate callsite is boring.  Multiplier increased to 2.3.
calleeNativeSizeEstimate=421
callsiteNativeSizeEstimate=85
benefit multiplier=2.3
threshold=195
Native estimate for function size exceeds threshold for inlining 42.1 > 19.5 (multiplier = 2.3)


Inline expansion aborted, inline not profitable
Inlining [000008] failed, so bashing STMT00002 to NOP

INLINER: during 'fgInline' result 'failed this call site' reason 'unprofitable inline' for 'JitBugReproduction.Program:Main()' calling 'JitBugReproduction.Program:PopcountWithCheck(long):long'
INLINER: during 'fgInline' result 'failed this call site' reason 'unprofitable inline'

The fundamental problem here is that reasoning about function calls (and thus properties) being constant at inlining time is somewhat expensive, because IL only has tokens in it, and the Jit has to ask the VM for an actual handle to the method to get useful information about it.

See #41692 (comment) or #38163 for some of the possible strategies that could help here. There are certainly possibilities for improvements in this area, but for now, AggressiveInlining is the tool to use.

[hypeartist]

@Tearth Adding [MethodImpl(MethodImplOptions.AggressiveInlining)] to PopLsb2 fixes the issue:

https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABABgAJiBGAOgCUBXAOwwEt8YaBhCfAB1YAbGFADKIgG6swMXAG4AsAChlTbB1x9sM8gAUIfADK5gAFVkZlAb2Xk7lAEyUqAdlv2bS+9+dJyDQQgmAHNyBEUvHztqP2IUcgBZbFYmAAoASncozyiohHIAXj0DY2AqVKpSUnSI3J98ov0jEwdUhyqarJ8AXy7vPvsY/0CQ4uay1ICg0IlsQQYYTMjsgajiF2d6ZjYOGgBJFigU3GlcGgANAA4kGgAhfFZac9R6WRgMQwgAdwtxDFvWBhUrN5otarlessfKtyABtBLvAAWEAAJnt+IJUgiMMi0RiAPJ8NhBM4AQWCwVguBOEhgB0EKRSwXSAF0YUMpqMmqVWpyZnMFks6jk6vZWAAzcipahbFjsTgHDBHJgnMBnK43e6Pfa4UQMPh8aAYGAooWiuwi83edabRhy3aK5Wq9XXO4PJ4vOhvD7fX7vAFAkGC8HmyFW8gwQS4GAw7yW8OUDZBmDkABkUuT5Dg5ConShdTDEK6kO6QA==

[Tearth]

@hypeartist yeah, that's exactly what I wrote in Analysis section (:

Thank you all for the explanation, good to know how does it work. For now, I will use AggressiveInlining which works perfectly in this case.

[AndyAyersMS]

For now, I will use AggressiveInlining which works perfectly in this case.

@Tearth thanks for the report. As others have commented, the strategy the jit uses currently for assessing inline candidates has various limitations. These limitations are not so easy to fix, as the aim is to find good inlines without needing to do a lot of detailed analysis.

Marking this as future as we intend to revisit this entire area someday.

[tannergooding]

Had discussed this briefly with @AndyAyersMS on Discord, but I'll comment here as well...

It might be interesting to see if we could cache tokens for certain "well-known" intrinsic methods. In particular, I think it would be useful for {Isa}.IsSupported, Type.GetTypeFromHandle, and Type.op_Equality as these represent patterns that can lead to large amounts of dead code optimization, often get used in decently sized if/else chains, and are the root cause of many of the "this isn't inlined when I expected it to be" bugs like this one.

[AndyAyersMS]

@tannergooding with the repro up top and a checked jit I hit an assert in morph while hashing the HW intrinsic node...

Morphing BB01 of 'PopLsbTest.Program:Main_47434()'
               [000014] ------------              *  HWINTRINSIC long    ResetLowestSetBit
               [000013] ------------              \--*  CNS_INT   long   100

Assert failure(PID 18236 [0x0000473c], Thread: 31860 [0x7c74]): Assertion failed 'type < CORINFO_TYPE_COUNT' in 'PopLsbTest.Program:Main_47434()' during 'Morph - Global' (IL size 27)

    File: C:\repos\runtime2\src\coreclr\jit\ee_il_dll.hpp Line: 273

Seems like the aux type field is not set?

[tannergooding]

That would be related to #50832.

We don't set the aux type for most node kinds. I must have missed one of the cases that always retrieves the kind and where it needs to handle CORINFO_TYPE_UNDEF to TYPE_UNKNOWN

[AndyAyersMS]

gtHashValue in gentree:

#ifdef FEATURE_HW_INTRINSICS
                case GT_HWINTRINSIC:
                    hash += tree->AsHWIntrinsic()->gtHWIntrinsicId;
                    hash += tree->AsHWIntrinsic()->gtSIMDBaseType;
                    hash += tree->AsHWIntrinsic()->gtSIMDSize;
                    hash += tree->AsHWIntrinsic()->GetAuxiliaryType();
                    break;
#endif // FEATURE_HW_INTRINSICS

[tannergooding]

Should be resolved with #51627