Value array stack alloc alt

[AndyAyersMS]

No description provided.

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[hez2010]

This line is a correctness fix. We were checking the index tree using the arr complexity.

[AndyAyersMS]

Ok, thanks. We should probably PR things like that separately.

[AndyAyersMS]

Guess it's not actually a functional change so no need for a separate PR

    const int MAX_ARR_COMPLEXITY   = 4;
    const int MAX_INDEX_COMPLEXITY = 4;

[hez2010]

Yeah I think it's a copy-paste mistake.

[AndyAyersMS]

I can't repro the x86 failure. Haven't looked into the R2R issues yet.

[AndyAyersMS]

One flaw with this approach is that the explicit zero inits are getting dead-coded by early liveness, because there are no evident uses of the array temp. We can fix this by using a custom new helper which takes the array address as an argument. And this introduces a dependence between the zero init and the new helper, which was absent before.

That is, instead of the current

   lArray = 0;
   pArray = new(type, size);
          = pArray[i];

we produce

   lArray = 0;
   pArray = new(type, size, &lArray);
          = pArray[i];

and (both) get later expanded to

   lArray = 0
   pArray = &lArray;
   pArray[0] = type;
   pArray[lenOffset] = size;
          = pArray[i];

The downside is we now have a jit helper with no runtime counterpart, which is a new concept, or we just tack on an extra arg to the existing helpers and hope it doesn't cause problems.

[AndyAyersMS]

This should show a decent set of SPMI diffs (though with 160ish misses in coreclr_tests we can remedy once we do yet another round of collection). Most of the diffs are now just extra prolog zeroing as the code shape in the method body is generally the same. Only rarely can we propagate through an array element, though I think we can improve on this some over time.

I think it's in good shape and we should seriously consider it as the way forward. We can either use this PR or merge back into #104906.

@hez2010, @jakobbotsch thoughts?

[AndyAyersMS]

For the R2R failures, it is failing on

TestRangeCheckElimination
Found:False
Not equal!
actual   = True
expected = False

This test seems to be checking what methods are inlineable across assembly boundaries, and evidently this method is now missing when it's expected to be in the allow list (I think the actual and expected labels are swapped here, the method is expected to be found in the list but isn't.)

Need to dig into this further. When I looked at it earlier I couldn't figure out where things were going wrong.

[hez2010]

I'm seeing some regressions like this:

 
 G_M59007_IG01:        ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, nogc <-- Prolog IG
-       sub      rsp, 40
+       sub      rsp, 24
 						;; size=4 bbWeight=1 PerfScore 0.25
 G_M59007_IG02:        ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
+       vxorps   xmm0, xmm0, xmm0
+       vmovdqu  xmmword ptr [rsp], xmm0
+       vmovdqu  xmmword ptr [rsp+0x04], xmm0
+       mov      rax, 0xD1FFAB1E      ; <unknown class>
+       mov      qword ptr [rsp], rax
+       mov      dword ptr [rsp+0x08], 2
        xor      eax, eax
-						;; size=2 bbWeight=1 PerfScore 0.25
+						;; size=39 bbWeight=1 PerfScore 4.83
 G_M59007_IG03:        ; bbWeight=1, epilog, nogc, extend
-       add      rsp, 40
+       add      rsp, 24
        ret      

It seems that we don't eliminate a dead stack allocated array. Is it expected?

[AndyAyersMS]

It seems that we don't eliminate a dead stack allocated array. Is it expected?

Yes, we will need some enhancements to detect cases like this.

[AndyAyersMS]

Pushed the changes over to #104906. So will close this.