Branches guarded by type checks not being trimmed in generic types/methods

[stephentoub]

Consider this code:

using System;

public class Test
{
    public static void Main() => Bar<bool>.Foo();
}

internal class Bar<T>
{
    public static void Foo()
    {
        if (typeof(T) == typeof(bool))
        {
            Console.WriteLine("bool");
        }
        else if (typeof(T) == typeof(int))
        {
            Console.WriteLine("int");
        }
    }
}

Since this code should be specialized per value type, I was expecting Foo to compile down to effectively "Console.WriteLine("bool")", but instead I get this:

; Assembly listing for method Bar`1[Boolean][System.Boolean]:Foo()
; Emitting BLENDED_CODE for X64 CPU with AVX
; optimized code
; rsp based frame
; fully interruptible
; Final local variable assignments
;
;  V00 tmp0         [V00,T00] (  2,   4  )     ref  ->  rsi
;  V01 tmp1         [V01,T01] (  2,   2  )     ref  ->  rsi
;  V02 OutArgs      [V02    ] (  1,   1  )  lclBlk (32) [rsp+0x00]
;
; Lcl frame size = 32

G_M20354_IG01:
       56                   push     rsi
       4883EC20             sub      rsp, 32

G_M20354_IG02:
       48B9081BE1B9FB7F0000 mov      rcx, 0x7FFBB9E11B08
       E850BBF65E           call     CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE
       488BF0               mov      rsi, rax
       48B9081BE1B9FB7F0000 mov      rcx, 0x7FFBB9E11B08
       E83EBBF65E           call     CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE
       483BC6               cmp      rax, rsi
       751F                 jne      SHORT G_M20354_IG04
       48B9205728EFA5020000 mov      rcx, 0x2A5EF285720
       488B09               mov      rcx, gword ptr [rcx]
       48B850010CBAFB7F0000 mov      rax, 0x7FFBBA0C0150

G_M20354_IG03:
       4883C420             add      rsp, 32
       5E                   pop      rsi
       48FFE0               rex.jmp  rax

G_M20354_IG04:
       48B9081BE1B9FB7F0000 mov      rcx, 0x7FFBB9E11B08
       E80BBBF65E           call     CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE
       488BF0               mov      rsi, rax
       48B98099E1B9FB7F0000 mov      rcx, 0x7FFBB9E19980
       E8F9BAF65E           call     CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE
       483BC6               cmp      rax, rsi
       751F                 jne      SHORT G_M20354_IG06
       48B9285728EFA5020000 mov      rcx, 0x2A5EF285728
       488B09               mov      rcx, gword ptr [rcx]
       48B850010CBAFB7F0000 mov      rax, 0x7FFBBA0C0150

G_M20354_IG05:
       4883C420             add      rsp, 32
       5E                   pop      rsi
       48FFE0               rex.jmp  rax

G_M20354_IG06:
       4883C420             add      rsp, 32
       5E                   pop      rsi
       C3                   ret

; Total bytes of code 149, prolog size 5 for method Bar`1[Boolean][System.Boolean]:Foo()
; ============================================================

This is a simplification of code we have in the async/await infrastructure, for getting cached tasks for certain return types, and it used to produce the correct code on desktop... has something changed here recently? Is this not supposed to remove code in the manner I expected?

[jkotas]

cc @dotnet/jit-contrib

[stephentoub]

For reference, here's the method in corelib that led me to open this issue:
https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Runtime/CompilerServices/AsyncMethodBuilder.cs#L760
If I manually trim the method to what I'd expect for, say, T==bool, the throughput of calling an empty async Task<bool> method improves by ~15%.

[mikedn]

Hrm, this used to work. It looks like the type check is split into 2 separate statements and morph can no longer recognize this pattern:

lvaGrabTemp returning 1 (V01 tmp0) called for non-inline candidate call.


               [000009] ------------             *  stmtExpr  void  (IL 0x000...  ???)
               [000003] --CXG-------             |  /--*  call help ref    HELPER.CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE
               [000001] ------------ arg0        |  |  \--*  const(h)  long   0x7ffed9085500 class
               [000008] -ACXG-------             \--*  =         ref   
               [000007] D------N----                \--*  lclVar    ref    V01 tmp0         

    [ 2]  20 (0x014) call 0A000007
In Compiler::impImportCall: opcode is call, kind=0, callRetType is bool, structSize is 0
INLINER: during 'impMarkInlineCandidate' result 'failed this callee' reason 'noinline per IL/cached result' for 'Bar:Test():int:this' calling 'System.Type:op_Equality(ref,ref):bool'
INLINER: during 'impMarkInlineCandidate' result 'failed this callee' reason 'noinline per IL/cached result'

    [ 1]  25 (0x019) brfalse.s

               [000017] ------------             *  stmtExpr  void  (IL   ???...  ???)
               [000016] --CXG-------             \--*  jmpTrue   void  
               [000014] ------------                |  /--*  const     int    0
               [000015] --CXG-------                \--*  ==        int   
               [000011] --CXG-------                   \--*  call      int    System.Type.op_Equality
               [000010] ------------ arg0                 +--*  lclVar    ref    V01 tmp0         
               [000006] --CXG------- arg1                 \--*  call help ref    HELPER.CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE
               [000004] ------------ arg0                    \--*  const(h)  long   0x7ffed9085500 class

I suppose it could be caused by a change in the inlining code.

[mikedn]

Unfortunate side effect of dotnet/coreclr#7923
cc @AndyAyersMS

[AndyAyersMS]

Yeah, the transform done in fgMorphCall requires an intact tree. Unfortunately we have to split the trees like we do or risk generating bad code.

We clearly must be overstating the side effects of these operations if we're able to fold them all away, so perhaps we could get the side effects described more accurately during importation and then the tree split wouldn't happen. It might seem odd (or cause problems) to have a call without the GTF_CALL effect though.

Or, given how idiomatic this pattern is we might be able to walk back and find the matching part during morph. Not sure if this is viable though -- presumably if part of the pattern is now a local and there's just one use and the def is from the statement before.

Otherwise we could think about fixing this during value numbering maybe?

[russellhadley]

@JosephTremoulet how hard would it be to get this with our current VN?

[JosephTremoulet]

Hopefully it's as easy as adding CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE here... that should give the two helper calls the same value number, but the snippet above shows the equality test between them as a call... do we inline that call or something, or would value-numbering need to pattern-match it? It looks like the code in morph is expecting a GT_EQ...

[AndyAyersMS]

We know something special about Type.op_Equality and special-case its behavior (see tail end of fgMorphCall).

Looks like the easiest fix is to avoid spilling in the importer -- if we see a call to Type.op_Equality we can just suppress the eval stack spill, since we know that call won't modify a local.

We still might want to do the value-based opt too, in case this stuff ever gets spread out across statements.

[JosephTremoulet]

Actually, that code at the end of morph rewrites the call to op_Equality as a compare when it fires, so the value numbering approach does work. I'll send out a PR...

[AndyAyersMS]

The importer side stuff seems to be doing the trick too. Will do a bit more testing and also send out a PR.

Puzzled why when working on dotnet/coreclr#7923 I didn't see any diffs. This pattern is pretty common.

[JosephTremoulet]

Puzzled why when working on dotnet/coreclr#7923 I didn't see any diffs. This pattern is pretty common.

Maybe crossgen tends not to instantiate these methods with the triggering type arguments?