Fix value numbering when selecting a constant

[jakobbotsch]

When applying selectors, constants were special-cased to not require any
type casts. However this is wrong if a narrowing needs to be performed.

Fix #18235

I have fixed this problem by removing the special casing. The diff is easier to read without whitespace (there should only be deletions).

[dnfclas]

All CLA requirements met.

[jakobbotsch]

I was originally going to wait for green light in #18235 since it has already been assigned, however I was trying to familiarize myself with the JIT tools and the fix seemed so simple that I might as well submit it.

I am not 100% sure which parts of jit-diff to run exactly; I have run with --pmi --frameworks --tests, and there are no diffs except in the two added tests:

PMI Diffs for System.Private.CoreLib.dll, framework assemblies, assemblies in Z:\Programming\dotnet\coreclr\bin\tests\Windows_NT.x64.Release for x64 default jit
Summary:
(Lower is better)
Total bytes of diff: 10 (0,00 % of base)
    diff is a regression.
Top file regressions by size (bytes):
           5 : JIT\Regression\JitBlue\GitHub_18235\GitHub_18235_1\GitHub_18235_1.dasm (8,77 % of base)
           5 : JIT\Regression\JitBlue\GitHub_18235\GitHub_18235_2\GitHub_18235_2.dasm (8,33 % of base)
2 total files with size differences (0 improved, 2 regressed), 2818 unchanged.
Top method regessions by size (bytes):
           5 : JIT\Regression\JitBlue\GitHub_18235\GitHub_18235_1\GitHub_18235_1.dasm - Program:Main():int
           5 : JIT\Regression\JitBlue\GitHub_18235\GitHub_18235_2\GitHub_18235_2.dasm - Program:Main():int
2 total methods with size differences (0 improved, 2 regressed), 342009 unchanged.
Completed analysis in 179,70s

The diffs on the test cases are:

 ; Assembly listing for method C0:.ctor():this
 ; Emitting BLENDED_CODE for X64 CPU with AVX
 ; optimized code
 ; rsp based frame
 ; partially interruptible
 ; Final local variable assignments
 ;
 ;* V00 this         [V00    ] (  0,  0   )     ref  ->  zero-ref    this class-hnd
 ;# V01 OutArgs      [V01    ] (  1,  1   )  lclBlk ( 0) [rsp+0x00]  
 ;
 ; Lcl frame size = 0
 G_M41541_IG01:
 G_M41541_IG02:
        ret      
 ; Total bytes of code 1, prolog size 0 for method C0:.ctor():this
 ; ============================================================
 Unwind Info:
   >> Start offset   : 0x000000 (not in unwind data)
   >>   End offset   : 0xd1ffab1e (not in unwind data)
   Version           : 1
   Flags             : 0x00
   SizeOfProlog      : 0x00
   CountOfUnwindCodes: 0
   FrameRegister     : none (0)
   FrameOffset       : N/A (no FrameRegister) (Value=0)
   UnwindCodes       :
 ; Assembly listing for method Program:Main():int
 ; Emitting BLENDED_CODE for X64 CPU with AVX
 ; optimized code
 ; rsp based frame
 ; partially interruptible
 ; Final local variable assignments
 ;
 ;* V00 loc0         [V00    ] (  0,  0   )     ref  ->  zero-ref    class-hnd exact
-;  V01 loc1         [V01,T02] (  5,  5   )    long  ->  rax        
+;  V01 loc1         [V01,T01] (  3,  3   )    long  ->  rax        
 ;  V02 OutArgs      [V02    ] (  1,  1   )  lclBlk (32) [rsp+0x00]  
-;  V03 tmp1         [V03,T00] (  7, 14   )     ref  ->  rax         class-hnd exact
-;  V04 cse0         [V04,T01] (  8,  8   )     int  ->  rax        
+;  V03 tmp1         [V03,T00] (  5, 10   )     ref  ->  rax         class-hnd exact
 ;
 ; Lcl frame size = 40
 G_M40942_IG01:
        sub      rsp, 40
 G_M40942_IG02:
        mov      rcx, 0xD1FFAB1E
        call     CORINFO_HELP_NEWSFAST
        mov      byte  ptr [rax+8], -1
        movzx    rax, byte  ptr [rax+8]
+       or       eax, 0x3E8
        movsxd   rax, eax
        cmp      rax, 0x3FF
        je       SHORT G_M40942_IG04
        xor      eax, eax
 G_M40942_IG03:
        add      rsp, 40
        ret      
 G_M40942_IG04:
        mov      eax, 100
 G_M40942_IG05:
        add      rsp, 40
        ret      
-; Total bytes of code 55, prolog size 4 for method Program:Main():int
+; Total bytes of code 60, prolog size 4 for method Program:Main():int
 ; ============================================================
 Unwind Info:
   >> Start offset   : 0x000000 (not in unwind data)
   >>   End offset   : 0xd1ffab1e (not in unwind data)
   Version           : 1
   Flags             : 0x00
   SizeOfProlog      : 0x04
   CountOfUnwindCodes: 1
   FrameRegister     : none (0)
   FrameOffset       : N/A (no FrameRegister) (Value=0)
   UnwindCodes       :
     CodeOffset: 0x04 UnwindOp: UWOP_ALLOC_SMALL (2)     OpInfo: 4 * 8 + 8 = 40 = 0x28
 ; Assembly listing for method Program:.ctor():this
 ; Emitting BLENDED_CODE for X64 CPU with AVX
 ; optimized code
 ; rsp based frame
 ; partially interruptible
 ; Final local variable assignments
 ;
 ;* V00 this         [V00    ] (  0,  0   )     ref  ->  zero-ref    this class-hnd
 ;# V01 OutArgs      [V01    ] (  1,  1   )  lclBlk ( 0) [rsp+0x00]  
 ;
 ; Lcl frame size = 0
 G_M48452_IG01:
 G_M48452_IG02:
        ret      
 ; Total bytes of code 1, prolog size 0 for method Program:.ctor():this

 ; Assembly listing for method Program:Main():int
 ; Emitting BLENDED_CODE for X64 CPU with AVX
 ; optimized code
 ; rsp based frame
 ; partially interruptible
 ; Final local variable assignments
 ;
 ;  V00 loc0         [V00,T00] (  1,  1   )  struct (24) [rsp+0x08]   do-not-enreg[SFB] must-init ld-addr-op
 ;* V01 loc1         [V01,T01] (  0,  0   )     int  ->  zero-ref   
 ;# V02 OutArgs      [V02    ] (  1,  1   )  lclBlk ( 0) [rsp+0x00]  
 ;
 ; Lcl frame size = 32
 G_M35631_IG01:
        push     rdi
        sub      rsp, 32
        vzeroupper 
        lea      rdi, [rsp+08H]
        mov      ecx, 6
        xor      rax, rax
        rep stosd 
 G_M35631_IG02:
        xor      rax, rax
        lea      rdx, bword ptr [rsp+08H]
        vxorps   xmm0, xmm0
        vmovdqu  qword ptr [rdx], xmm0
        mov      qword ptr [rdx+16], rax
-       mov      dword ptr [reloc classVar[0xd1ffab1e]], 0x10001
+       mov      dword ptr [reloc classVar[0xd1ffab1e]], 1
+       mov      eax, 100
 G_M35631_IG03:
        add      rsp, 32
        pop      rdi
        ret      
-; Total bytes of code 59, prolog size 22 for method Program:Main():int
+; Total bytes of code 64, prolog size 22 for method Program:Main():int

Any feedback is appreciated. I also have two questions:

• Should I run jit-diff without --pmi as well?
• Is there a way to get example diffs from jit-diff or jit-analyze directly? I have used git diff manually to produce the two diffs displayed above.

cc @dotnet/jit-contrib

[jakobbotsch]

By the way, I have tried to figure out why the special casing was there to begin with, but the code was added before .NET was open-sourced, so I cannot see if there is anything in the commit message.
Also, the same code is present in VNApplySelectorsAssignTypeCoerce:

coreclr/src/jit/valuenum.cpp

Line 2628 in e32feb0

if (isConstant && (elemTyp == genActualType(indType)))

I am unsure if this needs to be changed as well.

[AndyAyersMS]

Thanks for the prospective fix!

@briansull or @erozenfeld can help evaluate this change.
cc @dotnet/jit-contrib

jit-diffs without --pmi will find changes in prejitted code. Since large portions of the framework assemblies are prejitted it is worth running jit-diffs in that mode -- you will get some coverage that jit-diff --pmi can't get.

You don't need to run jit-diff in prejit mode over tests.

[AndyAyersMS]

Also I sent you a collaborator invite. We can assign issues to you once you accept.

[jakobbotsch]

I have accepted the invite. 👍
Also no crossgen diffs with this change:

Beginning Crossgen Diffs for System.Private.CoreLib.dll, framework assemblies
Completed Crossgen Diffs for System.Private.CoreLib.dll, framework assemblies in 197,17s
Analyzing diffs...
Crossgen Diffs for System.Private.CoreLib.dll, framework assemblies for x64 default jit
Summary:
(Lower is better)
Total bytes of diff: 0 (0,00 % of base)
0 total files with size differences (0 improved, 0 regressed), 130 unchanged.
0 total methods with size differences (0 improved, 0 regressed), 142234 unchanged.
Completed analysis in 125,58s

[jakobbotsch]

This does not fix the following case:

// Generated by Fuzzlyn on 2018-06-03 22:28:26
// Seed: 12435988927817691522
// Reduced from 70.9 KB to 0.4 KB
// Debug: Outputs 64447
// Release: Outputs -1089
class C0
{
    public short F1;
    public C0(short f1)
    {
        F1 = f1;
    }
}

class C1
{
    public C0 F9;
    public C1(C0 f9)
    {
        F9 = f9;
    }
}

public class Program
{
    public static void Main()
    {
        var vr61 = new C1(new C0(-20454));
        int vr63 = (ushort)(23487 | (long)vr61.F9.F1);
        System.Console.WriteLine(vr63);
    }
}

I am taking a look...

[jakobbotsch]

That case is probably the same as #18238. it looks like the cast disappears:

fgMorphTree BB01, stmt 8 (before)
               [000033] --CXG-------              *  CALL      void   System.Console.WriteLine
               [000032] ---XG------- arg0         \--*  CAST      int <- ushort <- long
               [000030] ---XG-------                 |  /--*  CAST      long <- int
               [000029] ---XG-------                 |  |  \--*  FIELD     short  F1
               [000028] ---XG-------                 |  |     \--*  FIELD     ref    F9
               [000027] ------------                 |  |        \--*  LCL_VAR   ref    V00 loc0         
               [000031] ---XG-------                 \--*  OR        long  
               [000026] ------------                    \--*  CAST      long <- int
               [000025] ------------                       \--*  CNS_INT   int    0x5BBF

Folding long operator with constant nodes into a constant:
               [000026] ------------              *  CAST      long <- int
               [000025] -----+------              \--*  CNS_INT   int    0x5BBF
Bashed to long constant:
               [000026] ------------              *  CNS_INT   long   0x5BBF

Assertion prop in BB01:
Copy     Assertion: V00 == V03 index=#03, mask=0000000000000004
               [000027] ------------              *  LCL_VAR   ref    V03 tmp2         
argSlots=1, preallocatedArgCount=4, nextSlotNum=4, outgoingArgSpaceSize=32

Sorting the arguments:
Deferred argument ('rcx'):
               [000026] -----+------              /--*  CNS_INT   int    0x5BBF
               [000031] ---XG+------              *  OR        int    ; this OR should be ushort
               [000030] ---XG+------              \--*  NOP       ushort
               [000029] ---XG+------                 \--*  IND       short 
               [000064] -----+------                    |  /--*  CNS_INT   long   8 field offset Fseq[F1]
               [000065] ---XG+------                    \--*  ADD       byref 
               [000028] ---XG+------                       \--*  IND       ref   
               [000066] -----+------                          |  /--*  CNS_INT   long   8 field offset Fseq[F9]
               [000067] -----+------                          \--*  ADD       byref 
               [000027] -----+------                             \--*  LCL_VAR   ref    V03 tmp2         
Replaced with placeholder node:
               [000068] ----------L-              *  ARGPLACE  int   

Shuffled argument table:    rcx 
fgArgTabEntry[arg 0 31.OR, 1 reg: rcx, align=1, lateArgInx=0, processed]

fgMorphTree BB01, stmt 8 (after)
               [000033] --CXG+------              *  CALL      void   System.Console.WriteLine
               [000026] -----+------              |  /--*  CNS_INT   int    0x5BBF
               [000031] ---XG+------ arg0 in rcx  \--*  OR        int   
               [000030] ---XG+------                 \--*  NOP       ushort
               [000029] ---XG+------                    \--*  IND       short 
               [000064] -----+------                       |  /--*  CNS_INT   long   8 field offset Fseq[F1]
               [000065] ---XG+------                       \--*  ADD       byref 
               [000028] ---XG+------                          \--*  IND       ref   
               [000066] -----+------                             |  /--*  CNS_INT   long   8 field offset Fseq[F9]
               [000067] -----+------                             \--*  ADD       byref 
               [000027] -----+------                                \--*  LCL_VAR   ref    V03 tmp2         

[briansull]

These changes Look Good

[erozenfeld]

LGTM

[erozenfeld]

By the way, I have tried to figure out why the special casing was there to begin with, but the code was added before .NET was open-sourced, so I cannot see if there is anything in the commit message.
Also, the same code is present in VNApplySelectorsAssignTypeCoerce

I found that commit (by @briansull in 2013). This code was in the original versions of VNApplySelectorsAssignTypeCoerce and VNApplySelectorsAssignTypeCoerce and the only comments about these methods in the commit message were:

- Added VNApplySelectorsTypeCheck to handle any type conversions for VNApplySelectors
- Added VNApplySelectorsAssignTypeCoerce to handle any type conversions for VNApplySelectorsAssign

[AndyAyersMS]

Suspect the test failures were unrelated. Let's retry them....

@dotnet-bot retest Windows_NT x64 Release CoreFX Tests
@dotnet-bot retest Windows_NT x64 Checked CoreFX Tests
@dotnet-bot retest OSX10.12 x64 Checked Innerloop Build and Test

@jakobbotsch can you add the standard license header to the new test cases, and also add them to the respective arm32 and arm64 test list files (see tests\arm*\tests.lst).

Once you add the tests to the list files, this PR and #18563 will have merge conflicts. @BruceForstall @jashook any thought on how to make those list files more automerge-friendly?

[jakobbotsch]

I've amended those changes.

[AndyAyersMS]

Thanks!

I'll sort out the test list issues over in my PR.