JIT: Use linear block order for MinOpts in LSRA

[amanasifkhalid]

Follow-up from #108086 (comment). Since LSRA shouldn't create cross-block live registers in MinOpts, LSRA's block ordering shouldn't matter, so just use the lexical order we have on-hand.

cc @dotnet/jit-contrib

[dotnet-policy-service]

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

[AndyAyersMS]

This is quite odd... probably worth figuring out what is going on in libraries pmi (likely very few minops methods, but still...)

[amanasifkhalid]

This is quite odd... probably worth figuring out what is going on in libraries pmi (likely very few minops methods, but still...)

I looked at the JitDisasm summary, and there are only a handful of MinOpts methods in libraries.pmi. The standout method is Microsoft.FSharp.Linq.RuntimeHelpers.LeafExpressionConverter:ConvExprToLinqInContext, which has 2459 basic blocks. I'm not able to run pin.exe on my AMD-based DevBox to take a closer look at the TP cost for this method, but perhaps there's some locality issue with using lexical order for super-large methods? For example, if we need to grab the predecessor of a block, the pred is less likely to be cached if we didn't use an RPO traversal, but this only becomes a problem if we have lots of basic blocks.

[amanasifkhalid]

Per discussion here, I'm going to try using the optimized layout all the time to see if we can get around some of the pitfalls of using a RPO sequence on its own.

[amanasifkhalid]

This is quite odd... probably worth figuring out what is going on in libraries pmi (likely very few minopts methods, but still...)

Sorry for getting back to you so late on this, but I finally got pin working thanks to Jakob. The bulk of the TP regression for the above method is coming from LinearScan::processKills. I did some rudimentary profiling, and both traversals call processKills the same number of times, but when using the lexical order, processKills, ends up doing a lot more work for this method: The inner loop runs for about 3.5x more iterations in total. I wonder if processing register kills in some order that doesn't match control flow resulted in some duplicated work -- perhaps the logic for killing registers can be simplified quite a bit for MinOpts, since we don't have cross-block live registers? @kunalspathak would you expect the register killing behavior to change in MinOpts when the block order changes?

[amanasifkhalid]

I took another look at the TP regression noted above, and it had to do with LinearScan::getKillSetForCall relying on Compiler::compFloatingPointUsed to determine if floating-point registers need to be killed or not. For this particular method, if LSRA iterates blocks in lexical order, it encounters a block that uses FP registers much earlier than it would've if it had used RPO, and because compFloatingPointUsed is global Compiler state, we end up unnecessarily processing FP register kills for hundreds of additional blocks. If we don't have cross-block live registers, then I think we can get away with setting this state on a per-block basis. With this approach, the TP diffs improve, and we only have one context in arm64 collections with diffs -- a trivial change in FP register allocation:

-            ldr     q16, [fp, #0xB8]	// [V12 tmp2]
-            str     q16, [fp, #0xD1FFAB1E]	// [V03 loc1]
-            ldr     q16, [fp, #0xC8]	// [V12 tmp2+0x10]
-            str     q16, [fp, #0xD1FFAB1E]	// [V04 loc2]
-            ldr     q16, [fp, #0xD1FFAB1E]	// [V03 loc1]
-            ldr     q17, [@RWD16]
-            tbl     v16.16b, {v16.16b}, v17.16b
-            str     q16, [fp, #0xD1FFAB1E]	// [V03 loc1]
+            ldr     q0, [fp, #0xB8]	// [V12 tmp2]
+            str     q0, [fp, #0xD1FFAB1E]	// [V03 loc1]
+            ldr     q0, [fp, #0xC8]	// [V12 tmp2+0x10]
+            str     q0, [fp, #0xD1FFAB1E]	// [V04 loc2]
+            ldr     q0, [fp, #0xD1FFAB1E]	// [V03 loc1]
+            ldr     q1, [@RWD16]
+            tbl     v0.16b, {v0.16b}, v1.16b
+            str     q0, [fp, #0xD1FFAB1E]	// [V03 loc1]

I also noticed that we were calling findPredBlockForLiveIn unnecessarily in MinOpts; for methods like the regressed one where some blocks have hundreds of predecessors, the pred iteration in findPredBlockForLiveIn can be expensive, so I've moved this logic onto the "enregister local vars" path.

@kunalspathak could you PTAL? Thanks!

[kunalspathak]

Changes LGTM and it was good catch regarding findPredBlockForLiveIn(). I was hoping to see asmdiff for MinOpts scenario with this change but how is it a zero diff?

[amanasifkhalid]

I was hoping to see asmdiff for MinOpts scenario with this change but how is it a zero diff?

If we don't have registers live across blocks, then LSRA will never need to use a block's "ideal" predecessor to identify incoming live registers, so skipping findPredBlockForLiveIn is expected to be a no-diff change. As for the change to compFloatingPointUsed, we can now skip killing FP registers more often, though maybe we don't have heavy use of FP registers intermingled with method calls within one basic block often enough, such that FP register pressure was all that big of a deal in MinOpts? It sounds like a narrow enough case that suggests this wouldn't improve codegen all that often.

[jakobbotsch]

As I understand it compFloatingPointUsed is meant to be set before we get to LSRA. What is the case where it is not set and then gets set later? I think it would be better to have some blockUsedFloatingPoint variable that is deterministically set for each block and that LSRA can use for decisions that involve only the particular block.

[kunalspathak]

Ideally yes and we should audit the cases where that is not happening. I remember running into this a while back and had to rely on it only after buildIntervals() where it was guaranteed to set correctly.

[amanasifkhalid]

In LinearScan::BuildDef, we have a check that sets compFloatingPointUsed to true if the tree we're building the definition for can't use integer registers. I believe you're understanding of compFloatingPointUsed is correct -- LSRA doesn't seem to be using it all that judiciously, hence this PR. I'll try your suggestion in a follow-up.

[amanasifkhalid]

In LinearScan::BuildDef, we have a check that sets compFloatingPointUsed to true if the tree we're building the definition for can't use integer registers.

We duplicate this check in LinearScan::identifyCandidates, so we have a few spots in LSRA to clean up.

[jakobbotsch]

The problem with this is that it is going to come with TP regressions when someone decides to set compFloatingPointUsed correctly in the future. That's why I would like it to be explicit if we are changing LSRA to make "block-local" decisions in some cases where that is ok.

[jakobbotsch]

It looks like the only place is when we create kills for calls. So likely we could just have some bool needsToKillFloatRegs that LSRA uses for this purpose, and that is block-local in MinOpts and global (using compFloatingPointUsed) in FullOpts. It would probably come with even better MinOpts TP improvements.

[amanasifkhalid]

The problem with this is that it is going to come with TP regressions when someone decides to set compFloatingPointUsed correctly in the future. That's why I would like it to be explicit if we are changing LSRA to make "block-local" decisions in some cases where that is ok.

That's valid, I'll open a follow-up PR for this.

[kunalspathak]

I was hoping to see asmdiff for MinOpts scenario with this change but how is it a zero diff?

If we don't have registers live across blocks, then LSRA will never need to use a block's "ideal" predecessor to identify incoming live registers, so skipping findPredBlockForLiveIn is expected to be a no-diff change. As for the change to compFloatingPointUsed, we can now skip killing FP registers more often, though maybe we don't have heavy use of FP registers intermingled with method calls within one basic block often enough, such that FP register pressure was all that big of a deal in MinOpts? It sounds like a narrow enough case that suggests this wouldn't improve codegen all that often.

I meant the change in setBlockSequence() should have shown asmdiffs in MinOpts?

[amanasifkhalid]

I meant the change in setBlockSequence() should have shown asmdiffs in MinOpts?

If registers aren't live across blocks, then the order of blocks allocated shouldn't affect codegen, right?