JIT: morph overly aggressive in killing local assertions

[AndyAyersMS]

Checking DefinesLocal is not sufficient to establish that a tree is a local def. One also needs to check for GTF_VAR_DEF.

Because of this, morph ends up killing off local assertions for variables at uses as well as defs. Not surprisingly, this limits the usefulness of local assertion prop.

https://github.com/dotnet/coreclr/blob/344004681ace2b49dbc40d83ce422f81aa7efcb5/src/jit/morph.cpp#L15183-L15193

fgMorphTree BB01, stmt 9 (before)
               [000025] n-----------              /--*  OBJ(24)   struct
               [000024] ------------              |  \--*  ADDR      byref 
               [000023] ------------              |     \--*  LCL_VAR   struct V02 tmp2         
               [000081] -A----------              *  ASG       struct (copy)
               [000079] D-----------              \--*  LCL_VAR   struct V04 tmp4         

The assignment [000025] using V02 removes: Copy     Assertion: V02 == V01

Fixing this shows some nice wins in System.Linq.Parallel:

PMI Diffs for System.Private.CoreLib.dll, framework assemblies for x64 default jit
Summary:
(Lower is better)
Total bytes of diff: -53442 (-0.14% of base)
    diff is an improvement.
Top file regressions by size (bytes):
          60 : CommandLine.dasm (0.02% of base)
          24 : System.Net.Sockets.dasm (0.01% of base)
          14 : System.Security.Cryptography.Cng.dasm (0.01% of base)
          13 : System.Reflection.Metadata.dasm (0.00% of base)
           8 : System.IO.FileSystem.dasm (0.01% of base)
Top file improvements by size (bytes):
      -47472 : System.Linq.Parallel.dasm (-4.38% of base)
       -2929 : Microsoft.CodeAnalysis.CSharp.dasm (-0.07% of base)
       -1427 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.03% of base)
       -1150 : System.Private.Xml.dasm (-0.03% of base)
        -283 : Microsoft.CodeAnalysis.dasm (-0.02% of base)
24 total files with size differences (16 improved, 8 regressed), 105 unchanged.
Top method regressions by size (bytes):
          60 (16.48% of base) : CommandLine.dasm - ErrorExtensions:ToParserResult(ref,struct):ref
          41 (100.00% of base) : System.Private.CoreLib.dasm - DefaultDecoder:GetChars(ref,int,int,ref,int):int:this (1 base, 2 diff methods)
          34 ( 3.23% of base) : System.Security.Cryptography.Algorithms.dasm - KeyFormatHelper:WriteEncryptedPkcs8(struct,struct,ref,ref):ref
          34 ( 3.23% of base) : System.Security.Cryptography.Cng.dasm - KeyFormatHelper:WriteEncryptedPkcs8(struct,struct,ref,ref):ref
          15 ( 2.63% of base) : System.Reflection.Metadata.dasm - MetadataReader:CalculateMethodDefTreatmentAndRowId(struct):int:this
Top method improvements by size (bytes):
       -1443 (-49.69% of base) : System.Linq.Parallel.dasm - UnaryQueryOperator`2:.ctor(ref):this (10 methods)
       -1443 (-50.03% of base) : System.Linq.Parallel.dasm - UnaryQueryOperator`2:.ctor(ref,bool):this (10 methods)
       -1200 (-27.45% of base) : System.Linq.Parallel.dasm - ParallelEnumerable:TakeWhile(ref,ref):ref (10 methods)
       -1200 (-27.45% of base) : System.Linq.Parallel.dasm - ParallelEnumerable:SkipWhile(ref,ref):ref (10 methods)
       -1200 (-27.57% of base) : System.Linq.Parallel.dasm - ParallelEnumerable:Select(ref,ref):ref (10 methods)
Top method regressions by size (percentage):
          41 (100.00% of base) : System.Private.CoreLib.dasm - DefaultDecoder:GetChars(ref,int,int,ref,int):int:this (1 base, 2 diff methods)
          60 (16.48% of base) : CommandLine.dasm - ErrorExtensions:ToParserResult(ref,struct):ref
           7 ( 3.29% of base) : System.Net.Sockets.dasm - SocketPal:SendAsync(ref,ref,int,ref):int
          34 ( 3.23% of base) : System.Security.Cryptography.Algorithms.dasm - KeyFormatHelper:WriteEncryptedPkcs8(struct,struct,ref,ref):ref
          34 ( 3.23% of base) : System.Security.Cryptography.Cng.dasm - KeyFormatHelper:WriteEncryptedPkcs8(struct,struct,ref,ref):ref
Top method improvements by size (percentage):
        -150 (-50.51% of base) : System.Linq.Parallel.dasm - DecimalAverageAggregationOperator:.ctor(ref):this
        -150 (-50.51% of base) : System.Linq.Parallel.dasm - DecimalSumAggregationOperator:.ctor(ref):this
        -150 (-50.51% of base) : System.Linq.Parallel.dasm - DoubleAverageAggregationOperator:.ctor(ref):this
        -150 (-50.51% of base) : System.Linq.Parallel.dasm - DoubleSumAggregationOperator:.ctor(ref):this
        -150 (-50.51% of base) : System.Linq.Parallel.dasm - FloatAverageAggregationOperator:.ctor(ref):this
330 total methods with size differences (277 improved, 53 regressed), 193022 unchanged.

cc @dotnet/jit-contrib

[mikedn]

One also needs to check for GTF_VAR_DEF.

I think it's a bit different, that assertion propagation code is misusing DefinesLocal. This is intended to be used with a GT_ASG tree but that morph code seems to be calling it on every node it encounters.

[mikedn]

Actually this is likely an issue in DefinesLocal itself. It checks for GT_ASG but it also checks OperIsBlk. The later is probably a leftover from GT_COPYBLK days and should be removed.

[AndyAyersMS]

Yeah, that seems like a better fix. Then DefinesLocal actually does more or less what one expects.

It gives the same set of diffs as my prototype change to check flags.

[briansull]

It would be surprising that a function named DefinesLocal would be returning true for a use of a LCL_VAR rather than a def.