[RyuJit/ARM] Fix lsra BuildShiftRotate

[sandreenko]

Fix uses order for shift and rotate uses on arm arch.
Before it added shiftBy before source ad when they point to the same local variable we hit assert:
codegenlinear.cpp (749) - Assertion failed 'varDsc->lvRegNum == tree->gtRegNum'
for such example:

0001 ldloc.s 0x01 (source,  refPos 2, execution order 1, marked GTF_SPILL)
0002 ldloc.s 0x01 (shiftBy, refPos 1, execution order 2, marked GTF_SPILLED)
shr
0003 ldloc.s 0x01 (another load of this local variables that is marked as GTF_SPILLED and GTF_SPILL, that means it should reload the value and then spill if it is not already spilled).

0001 will spill the variable first, then 0002 will reload it and do not spill (because live interval is not marked as spill_after because we build them in the exection order).
then 0003 will find that the variable is not spilled yet (it is location is not 'STACK' and will try to spill it, but check that it is alive only in the currect register).

The right order is to reload after 0001(GTF_SPILLED) spill after 0002(GTF_SPILL) and reload at 0003(GTF_SPILLED).

Fix DevDiv_545497 and DevDiv_544980.

Note: It looks very fragile that lsra builds its own uses order instead if using the execution order.

[sandreenko]

@dotnet-bot test Windows_NT arm Cross Checked r2r Build and Test
@dotnet-bot test Windows_NT x86 Checked r2r

[sandreenko]

PR that added the previous implementation for arm #10145 (with the error).

[sandreenko]

@dotnet-bot test Windows_NT x86_arm_altjit Checked r2r

[sandreenko]

This pr shows the failure.

[sandreenko]

PTAL @dotnet/jit-contrib

[sandreenko]

@dotnet-bot test Windows_NT arm Cross Checked r2r Build and Test
@dotnet-bot test Windows_NT x86 Checked r2r
@dotnet-bot test Windows_NT x86_arm_altjit Checked r2r
@dotnet-bot test Windows_NT arm Cross Checked r2r_jitstress1 Build and Test
@dotnet-bot test Windows_NT arm Cross Checked r2r_jitstress2 Build and Test
@dotnet-bot test Windows_NT arm Cross Checked jitstress2 Build and Test

[CarolEidt]

Note: It looks very fragile that lsra builds its own uses order instead if using the execution order.

Actually, it's not fidelity with the execution order of the nodes that matters. The "execution order" (which is basically meaningless for lclVar uses) may not be the same as the operand order in the node. What matters is that the order of uses modeled by LSRA must be the same as the order of uses used by the code generator. Both LSRA and codegen should be handling the uses according to their canonical order.

It would be costly for LSRA to use the operand iterator, as it frequently needs to special handle specific operands. One could add an assertion pass after BuildNode that checks that its uses are correctly ordered.

As an aside, it used to be that both LSRA and codegen would pay attention to the GTF_REVERSE_OPS flag. After Rationalizer was changed to remove the flag, the code in LSRA wasn't updated to eliminate its use (see #16528).

[CarolEidt]

LGTM

[sandreenko]

This PR causes regression in Windows_NT.arm.Debug\JIT\CodeGenBringUpTests\Rotate (wrong result) with JitStress=2. I am investigating.

[sandreenko]

Ok, there was a special handling for GT_LSH_HI and GT_RSH_LO that allowed this test to pass, but this handling was not completely right , because if you set jitdump than the jit hits asserts:

set complus_jitdump=rol64_16
old version:
C:\Users\seandree>"Y:\bin\tests\Windows_NT.arm.Debug\JIT\CodeGenBringUpTests\Rotate\Rotate.cmd"

Assert failure(PID 6000 [0x00001770], Thread: 6976 [0x1b40]): Assertion failed 'OperIsMultiRegOp()' in 'Test:rol64_16(long):long' (IL size 50)

    File: c:\git\coreclr\src\jit\gentree.cpp Line: 822
    Image: Y:\bin\tests\Windows_NT.arm.Debug\Tests\Core_Root\CoreRun.exe

current version:
C:\Users\seandree>"Y:\bin\tests\Windows_NT.arm.Debug\JIT\CodeGenBringUpTests\Rotate\Rotate.cmd">bad.txt
BEGIN EXECUTION
 "Y:\bin\tests\Windows_NT.arm.Debug\Tests\Core_Root\corerun.exe" Rotate.exe
Expected: 100
Actual: -1
END EXECUTION - FAILED
FAILED

[CarolEidt]

@sandreenko - I'm not sure I understand what's failing and what's not, but there's a problem with the code in GenTree::GetRegisterDstCount(). On ARM, a GenTreeCopyOrReload is a multi-reg node, but it's not a GenTreeMultiRegOp. So, if you have a copy or reload above a non-call multi-reg node, you will get that assert. I have a fix in my WIP PR #16517 - look at the changes to gentree.cpp.

However, I believe that all the uses of GetRegisterDstCount() are in DEBUG code, so that doesn't seem like it would cause your failures.

At some point I would like to see us clean up the handling of multi-reg nodes in the GenTree hierarchy.

[sandreenko]

PR was updated, so I returned the special handling for arm32 GT_LSH_HI and GT_RSH_LO.

[sandreenko]

@dotnet-bot test Windows_NT arm Cross Checked r2r Build and Test
@dotnet-bot test Windows_NT x86 Checked r2r
@dotnet-bot test Windows_NT x86_arm_altjit Checked r2r
@dotnet-bot test Windows_NT arm Cross Checked r2r_jitstress1 Build and Test
@dotnet-bot test Windows_NT arm Cross Checked r2r_jitstress2 Build and Test
@dotnet-bot test Windows_NT arm Cross Checked jitstress2 Build and Test

[sandreenko]

@sandreenko - I'm not sure I understand what's failing and what's not, but there's a problem with the code in GenTree::GetRegisterDstCount(). On ARM, a GenTreeCopyOrReload is a multi-reg node, but it's not a GenTreeMultiRegOp. So, if you have a copy or reload above a non-call multi-reg node, you will get that assert. I have a fix in my WIP PR #16517 - look at the changes to gentree.cpp.

Thank you, I was trying to compare master output with my branch results and hit this assert in the master version in debug.
The issue that I had in my branch was in GT_LSH_HI that needed the special handling to set isDelayFree flags on its operands.

[sandreenko]

Also this PR fixes DevDiv_544980

[CarolEidt]

I think it would be more correct to say that they have uses that interfere with the def (uses are always considered to interfere with each other).
Also, please put the note below the arguments as per the function header guidelines.
Otherwise, LGTM

[sandreenko]

Thank you, fixed.