JIT: checked-release asmdiffs for libraries_tests.run.linux.arm64.Release.mch

[AndyAyersMS]

4 diffs overall, perhaps all the same or in a similar method

See https://dev.azure.com/dnceng-public/public/_build/results?buildId=765225&view=results

[11:10:01] ISSUE: <ASM_DIFF> main method 617927 of size 380 differs
[11:03:31] ISSUE: <ASM_DIFF> main method 710437 of size 380 differs
[11:03:31] ISSUE: <ASM_DIFF> main method 710603 of size 380 differs
[11:03:31] ISSUE: <ASM_DIFF> main method 710588 of size 380 differs

[dotnet-policy-service]

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

[JulieLeeMSFT]

@amanasifkhalid, PTAL.

[amanasifkhalid]

I'm seeing behavioral diffs in the ldp peephole between Checked and Release JITs. Here it is kicking in for the Checked JIT:

ldr     w1, [x19, #0x28]
and     w1, w1, #0x7FFFFFFF
ldp     w0, w2, [x19, #0x2C]
mul     w25, w1, w0
add     w1, w25, w2
add     w1, w1, #21
ldr     w0, [x19, #0x4C]
ldr     w2, [x19, #0x34]
sub     w0, w0, w2
cmp     w0, w1

but not for the Release JIT:

ldr     w1, [x19, #0x28]
and     w1, w1, #0x7FFFFFFF
ldr     w0, [x19, #0x2C]
mul     w25, w1, w0
ldr     w1, [x19, #0x30]
add     w1, w25, w1
add     w1, w1, #21
ldr     w0, [x19, #0x4C]
ldr     w2, [x19, #0x34]
sub     w0, w0, w2
cmp     w0, w1

Here's another instance where the register pairing is different. Checked JIT:

ldr     w0, [x19, #0x28]
and     w0, w0, #0x7FFFFFFF
ldp     w1, w2, [x19, #0x2C]
mul     w0, w0, w1
sxtw    w28, w0
add     w0, w28, w2
add     w1, w0, #21
ldr     w0, [x19, #0x4C]
ldr     w2, [x19, #0x34]
sub     w0, w0, w2
cmp     w0, w1

Release JIT:

ldr     w0, [x19, #0x28]
and     w0, w0, #0x7FFFFFFF
ldr     w1, [x19, #0x2C]
mul     w0, w0, w1
sxtw    w28, w0
ldp     w0, w1, [x19, #0x30]
add     w0, w28, w0
add     w2, w0, #21
ldr     w0, [x19, #0x4C]
sub     w0, w0, w1
cmp     w0, w2

[amanasifkhalid]

I suspect we aren't maintaining our DFS/loop data structures consistently between Debug/Release builds, and #105695 exposed this inconsistency (for example, by the time we get to Lowering::IsStoreToLoadForwardingCandidateInLoop, we may or may not need to recompute these data structures depending on the build type, thus giving us different codegen). Currently digging into this.

[amanasifkhalid]

Fortunately, the problem is more localized than I initially thought: In Lowering::TryMakeIndirsAdjacent, we have this flag check:

if ((prevIndir->gtLIRFlags & LIR::Flags::Mark) != 0)
{
    JITDUMP("Previous indir is part of the data flow of current indir\n");
    UnmarkTree(indir);
    return false;
}

Right before that, there's some Debug code for dumping LIR:

#ifdef DEBUG
    bool     isClosed;
    GenTree* startDumpNode = BlockRange().GetTreeRange(prevIndir, &isClosed).FirstNode();
    GenTree* endDumpNode   = indir->gtNext;

    // Dumping logic below

GetTreeRange sets the flag LIR::Flags::Mark on prevIndir->gtLIRFlags, and then calls GetMarkedRange, which will always unset the flag here.

So in Debug/Checked builds, if a node has this flag set upon entry into Lowering::TryMakeIndirsAdjacent, it will be unset by Debug-only logic, and the ldp/stp transformation may continue, while the Release JIT will always bail. @jakobbotsch I'm guessing the Release logic is correct, and the simplest solution is for the Debug snippet above to save/restore this flag on prevIndir, right? This approach fixes the diffs.

[jakobbotsch]

There is an invariant for LIR that LIR::Flags::Mark is not set on nodes so that functions can use it for a cheap "in set" operation. So on entry to TryMakeIndirsAdjacent we should not have this flag set on any node.
The problem here is a missing UnmarkTree call in the IsStoreToLoadForwardingCandidateInLoop check I added the other day in #105695. You can see that the other exits do have it.

[jakobbotsch]

This is pretty error prone -- can you switch it to use jitstd::utility::scoped_code to avoid that kind of mistake in the future?

[amanasifkhalid]

Thanks for pointing that out!

This is pretty error prone -- can you switch it to use jitstd::utility::scoped_code to avoid that kind of mistake in the future?

Sure.