JIT: handle case where we are cloning adjacent sibling loops

[AndyAyersMS]

We can sometimes see adjacent sibling loops (where L1.bottom == L2.head)
and if so, cloning L1 will repurpose L1.bottom and so leave L2 in
an inconsistent state.

Detect this case and give L2 a temporary preheader before cloning L1.
Revoke preheader status of L2 if we then go on to clone L2, as cloning
does not preserve preheaders currently.

Fixes #70569.

[ghost]

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

Issue Details

---

We can sometimes see adjacent sibling loops (where L1.bottom == L2.head)
and if so, cloning L1 will repurpose L1.bottom and so leave L2 in
an inconsistent state.

Detect this case and give L2 a temporary preheader before cloning L1.
Revoke preheader status of L2 if we then go on to clone L2, as cloning
does not preserve preheaders currently.

Fixes #70569.

Author:	AndyAyersMS
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[AndyAyersMS]

@BruceForstall PTAL
cc @dotnet/jit-contrib

Not the most elegant solution, I'll admit, but the "temporary" preheader may get reinstated later on so the block may end up being useful.

No diffs. The original issue was from random PGO stress in libraries-pgo.

Final local flow graph for the example over in #70596 below, showing both siblings successfully cloned. Note RBO gets rid of the in-loop type test in the two fast loops (but not in the two slow ones, as on the slow path the in-loop type test is not dominated by cloning condition type test; control can also reach the slow loop with a null ref).

[AndyAyersMS]

/azp run runtime-coreclr superpmi-diffs

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[AndyAyersMS]

Installer test hit some kind of timing/permissions issue. Will retry.

  Tests succeeded: D:\a\_work\1\s\artifacts\bin\Microsoft.NET.HostModel.AppHost.Tests\Release\net7.0\Microsoft.NET.HostModel.AppHost.Tests.dll [net7.0|x64]
D:\a\_work\1\s\.packages\microsoft.dotnet.arcade.sdk\7.0.0-beta.22310.1\tools\VSTest.targets(55,5): error MSB3491: Could not write lines to file "D:\a\_work\1\s\artifacts\log\Release\Microsoft.NET.HostModel.ComHost.Tests_net7.0_x64.log". The process cannot access the file 'D:\a\_work\1\s\artifacts\log\Release\Microsoft.NET.HostModel.ComHost.Tests_net7.0_x64.log' because it is being used by another process. [D:\a\_work\1\s\src\installer\tests\Microsoft.NET.HostModel.Tests\Microsoft.NET.HostModel.ComHost.Tests\Microsoft.NET.HostModel.ComHost.Tests.csproj]
##[error].packages\microsoft.dotnet.arcade.sdk\7.0.0-beta.22310.1\tools\VSTest.targets(55,5): error MSB3491: (NETCORE_ENGINEERING_TELEMETRY=Build) Could not write lines to file "D:\a\_work\1\s\artifacts\log\Release\Microsoft.NET.HostModel.ComHost.Tests_net7.0_x64.log". The process cannot access the file 'D:\a\_work\1\s\artifacts\log\Release\Microsoft.NET.HostModel.ComHost.Tests_net7.0_x64.log' because it is being used by another process.
  Running tests: D:\a\_work\1\s\artifacts\bin\Microsoft.NET.HostModel.Bundle.Tests\Release\net7.0\Microsoft.NET.HostModel.Bundle.Tests.dll [net7.0|x64]

[BruceForstall]

This feels like a hack. Ideally, I'd like to get rid of the 'head' concept; I think it's pretty broken/limiting.

Would it be cleaner to do some kind of pre-pass, say right after loop recognition, to create a new 'head' block (not a pre-header as defined) whenever a 'head' and 'bottom' coincide?

[BruceForstall]

Why not check optLoopTable[sibling].lpHead != b directly?

[BruceForstall]

Why check ambientLoop here? I guess it's so you can walk the sibling list. But can't the same situation exist for top-level adjacent loops, in which case you need to walk the entire loop table looking for loops without a parent.

[BruceForstall]

Suggested change

	JITDUMP("Clearing preader flag from " FMT_LP " and " FMT_BB "\n", loopInd);
	JITDUMP("Clearing preader flag from " FMT_LP " and " FMT_BB "\n", loopInd, h->bbNum);

[AndyAyersMS]

Yeah, I can look into doing a prepass instead. With a prepass we may alter the flow graph even if we're not going to clone anything, so it might lead to some diffs.

[AndyAyersMS]

Seems like maybe this fits into optCanonicalizeLoopNest so will look to add something there.

[AndyAyersMS]

Reworked this to run as part of optCanonicalizeLoop. Let me know if this seems more appealing.

I saw 4 methods with minor diffs in local SPMI.

[AndyAyersMS]

@BruceForstall I changed my approach here. See what you think.

[BruceForstall]

I like this approach, as it happens during "normal" canonicalization, and establishes a new invariant for the loop table.

Can you add appropriate asserts to Compiler::fgDebugCheckLoopTable() to ensure this invariant?

I think we have an ordering requirement on the loop table, but I don't see that Compiler::fgDebugCheckLoopTable() enforces one. I.e., for two top-level loops l1 and l2, they must be disjoint. If the blocks of l1 precede l2, is l1 guaranteed to appear before (at a lower index) than l2 in the loop table? If so, there's no reason to loop over the entire table; for loop index loopInd, look at loopInd - 1 (or the first preceding top-level loop), and you're done. Something similar should apply for siblings, but I guess we don't have a way to walk backward in the sibling list.

One thing I'm a little worried about is it looks like we allow a child loop to share "top" with its parent:

        static bool lpContains(const unsigned*   blockNumMap,
                               const LoopDsc*    loop,
                               const BasicBlock* top,
                               const BasicBlock* bottom)
        {
            return (blockNumMap[loop->lpTop->bbNum] <= blockNumMap[top->bbNum]) &&
                   (blockNumMap[bottom->bbNum] < blockNumMap[loop->lpBottom->bbNum]);
        }

(<= instead of <).

Do you know if we canonicalize to prevent this?

[AndyAyersMS]

If the blocks of l1 precede l2, is l1 guaranteed to appear before (at a lower index) than l2 in the loop table?

I think this will be true right after loop recognition, but I don't think this remains true forever, we may reorder entire ranges of blocks later on based on profile data.

it looks like we allow a child loop to share "top" with its parent:

This is a thing canonicalization fixes: it unshares tops. Until recently, this was the only thing canonicalization did. So, it should only be true for a short time (between registering the loop and canonicalizing the loop).

I've now added a second kind of canonicalization (#70809, unshare top with non-loop backedges) and this PR adds a third kind (unshare bottom with head), and we really should add a fourth kind (unshare entry with non-loop backedges -- which instead I've worked around for now, see #70959). And possibly more: it seems like we could have a mid-entry loop whose entry is also the top of a nested loop. so we might want to unshare top and entry.

An alternative to explore is to do backedge canonicalization so that each loop has one actual backedge (from inside the loop to entry) and this might collapse some of the things we'd currently call nested do-while loops to be a single do-while with multiple exits. And we might want to "describe" the non-loop like things we bail on today, because pretending they're not loops seems to be causing some problems.

I'm a fan of the loop recognition advocated by Havlak though it might be there are more recent approaches that work out better.

All of this comes about because we are now cloning more general things that we still recognize as loops; previously we'd restrict ourselves to cloning only iterable loops and those are more restricted in shape.

[BruceForstall]

This is a thing canonicalization fixes: it unshares tops. Until recently, this was the only thing canonicalization did. So, it should only be true for a short time (between registering the loop and canonicalizing the loop).

Seems like we should change fgDebugCheckLoopTable to assert there are no shared tops, since it only gets called once canonicalization has finished.

[BruceForstall]

This comment still applies:

Can you add appropriate asserts to Compiler::fgDebugCheckLoopTable() to ensure this invariant?

but I'll go ahead and approve. We can add this later (if we remember).

[AndyAyersMS]

This comment still applies:

Can you add appropriate asserts to Compiler::fgDebugCheckLoopTable() to ensure this invariant?

but I'll go ahead and approve. We can add this later (if we remember).

Added to 7.0 so should be hard to forget: #71084