JIT: strengthen checking of the loop table #71184

AndyAyersMS · 2022-06-23T03:39:14Z

Add loop table checking to the post-phase list, conditional on whether the
table is expected to be valid.

Declare that the table is valid from the end of the find loops phase to the
end of the optimization phases.

Add checks that sibling loops are fully disjoint, no child shares top with its
parent, and all top-entry loops have at most one non-loop backedge.

Closes #71084.

Add loop table checking to the post-phase list, conditional on whether the table is expected to be valid. Declare that the table is valid from the end of the find loops phase to the end of the optimization phases. Add checks that sibling loops are fully disjoint, no child shares top with its parent, and all top-entry loops have at most one non-loop backedge. Closes dotnet#71084.

ghost · 2022-06-23T03:39:27Z

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

Issue Details

Add loop table checking to the post-phase list, conditional on whether the
table is expected to be valid.

Declare that the table is valid from the end of the find loops phase to the
end of the optimization phases.

Add checks that sibling loops are fully disjoint, no child shares top with its
parent, and all top-entry loops have at most one non-loop backedge.

Closes #71084.

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

AndyAyersMS · 2022-06-23T03:40:59Z

@BruceForstall PTAL
cc @dotnet/jit-contrib

This probably won't survive some of the more ambitious CI legs (like libraries-pgo).

Let's see if it gets through innerloop first.

BruceForstall

Looks great! A couple nits and one major question about how often the new checking actually gets invoked.

src/coreclr/jit/fgdiagnostic.cpp

src/coreclr/jit/optimizer.cpp

BruceForstall · 2022-06-23T05:55:25Z

src/coreclr/jit/phase.cpp

@@ -232,6 +232,7 @@ void Phase::PostPhase(PhaseStatus status)
            comp->fgDebugCheckLinks();
            comp->fgDebugCheckNodesUniqueness();
            comp->fgVerifyHandlerTab();
+            comp->fgDebugCheckLoopTable();


I love having this here. (In fact, I wrote this exact change back in December but never got it in :-( ).

IIRC, not too many of the optimization phases actually go down this code path, so it won't get run much before the loop table is marked as not valid. I tried expanding the number of optimization phases that were "true" phase object phases, and used post-phase processing, and they kept hitting cases where the loop table was invalid.

VN and hoisting are dependent on proper loop structure, so it would be good to be confident that things are ok there. Let me try converting them to true phases (along with ssa and early prop)

AndyAyersMS · 2022-06-23T22:37:40Z

I added proper post-phase checks for all (most?) of the phases that run between find loops and hoisting. This passes local SPMI.

Some of these newly updated phases may not really deserve dumping like we're doing now (eg fgFindOperOrder will at most reverse some subtrees but will also set costs). Currently there's no way to separate out post-phase dumping from checking.

I now see a few small SPMI diffs where we are no longer unmarking loops and so doing some alignment padding, because optOptimizeLayout is now run with loop table invalid. Presumably this might fix the issue I see in #71071, or at least related problems. I might also move the invalidation up earlier when optimizing, if the troublemaker is the update flow graph call we make in there.

AndyAyersMS · 2022-06-25T15:21:57Z

/azp run runtime-coreclr jitstress

azure-pipelines · 2022-06-25T15:22:11Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS · 2022-06-27T18:54:42Z

@BruceForstall this has evolved quite a bit since you approved, can you look again?

Main thing is that many more phases opt into the default post phase checks, including all the ones that I know of that are critically dependent on the loop table being valid.

In some cases this required adding/exposing change tracking within the phases.

AndyAyersMS · 2022-06-27T19:35:32Z

Also verified this addresses the AV hit in #71071 as we no longer try and unmark loops during the Optimize layout phase.

BruceForstall

It feels like we should have an assert that if a phase returns MODIFIED_NOTHING, we could actually verify that. Think: clone IR/lcl vars/EH/loop table, run phase, compare against cache. Yes, expensive and lots of new code.

BruceForstall · 2022-06-27T21:32:26Z

src/coreclr/jit/flowgraph.cpp

 #endif // DEBUG
+
+    return PhaseStatus::MODIFIED_NOTHING;


Shouldn't this be MODIFIED_EVERYTHING?

Sure. Currently dumping and checking are tied together. I was trying to avoid redundant seeming dumps, but I suppose we want the checks.

BruceForstall · 2022-06-27T21:36:22Z

src/coreclr/jit/lclvars.cpp

@@ -4590,6 +4592,8 @@ void Compiler::lvaMarkLocalVars()
        }
    }

+    bool addedLocal = false;


It might be simpler to cache lvaCount here and then check lvaCount against the cached value at the end, instead of interspersing addedLocal = true calls and hoping you didn't miss any place.

Yes, that would be more robust. Will do.

BruceForstall · 2022-06-27T21:39:11Z

src/coreclr/jit/optimizer.cpp

@@ -604,6 +604,8 @@ void Compiler::optClearLoopIterInfo()
        loop.lpConstInit = -1;
        loop.lpTestTree  = nullptr;
    }
+
+    return PhaseStatus::MODIFIED_NOTHING;


Well, NOTHING is a misnomer. So maybe you could at least add a comment at the definition of MODIFIED_NOTHING that says what counts as something and what counts as nothing?

BruceForstall · 2022-06-27T21:44:34Z

src/coreclr/jit/optimizer.cpp

@@ -7645,9 +7665,6 @@ void Compiler::optHoistCandidate(GenTree* tree, BasicBlock* treeBb, unsigned lnu
        return;
    }

-    // Create a loop pre-header in which to put the hoisted code.
-    fgCreateLoopPreHeader(lnum);


You can't move this down because the next check depends on the pre-header already being created.

BruceForstall · 2022-06-27T21:47:37Z

src/coreclr/jit/compiler.cpp

@@ -4840,11 +4840,13 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl

    if (opts.OptimizationEnabled())
    {
+        // xxx


Fix comment

AndyAyersMS · 2022-06-28T01:51:04Z

It feels like we should have an assert that if a phase returns MODIFIED_NOTHING, we could actually verify that. Think: clone IR/lcl vars/EH/loop table, run phase, compare against cache. Yes, expensive and lots of new code

We have some of this checking of this already, see Phase::Observations::Check. It seems to work pretty well in practice.

AndyAyersMS · 2022-06-28T19:06:20Z

@BruceForstall updated per your feedback.

kunalspathak · 2022-06-29T15:34:16Z

src/coreclr/jit/optimizer.cpp

@@ -6792,6 +6809,9 @@ void Compiler::optHoistThisLoop(unsigned lnum, LoopHoistContext* hoistCtxt, Basi
    defExec.Push(pLoopDsc->lpEntry);

    optHoistLoopBlocks(lnum, &defExec, hoistCtxt);
+
+    const unsigned numHoisted = pLoopDsc->lpHoistedFPExprCount + pLoopDsc->lpHoistedExprCount;
+    return pLoopDsc->lpHoistAddedPreheader || (numHoisted > 0);


Sorry for getting back late, but what is the scenario where lpHoistAddedPreheader == true but numHoisted == 0?

I know that there is one scenario:

runtime/src/coreclr/jit/optimizer.cpp

Lines 7674 to 7680 in 7af23d4

if (!BasicBlock::sameTryRegion(optLoopTable[lnum].lpHead, treeBb))

{

JITDUMP(" ... not hoisting in " FMT_LP ", eh region constraint (pre-header try index %d, candidate " FMT_BB

" try index %d\n",

lnum, optLoopTable[lnum].lpHead->bbTryIndex, treeBb->bbNum, treeBb->bbTryIndex);

return;

}

But in that case, we should just return false that no hoisting was done?

Yes, there were a few cases in SPMI where we created a preheader and then failed the eh check.

Since we've modified the flow graph (even though we might not have hoisted anything) we need to return true.

kunalspathak · 2022-06-29T16:01:43Z

src/coreclr/jit/compiler.h

@@ -5958,17 +5960,10 @@ class Compiler

    // Do hoisting of all loops nested within loop "lnum" (an index into the optLoopTable), followed
    // by the loop "lnum" itself.
-    //
-    // "m_pHoistedInCurLoop" helps a lot in eliminating duplicate expressions getting hoisted


Any reason this comment was deleted?

Looks like an inadvertent edit -- at one point I had modified the signature of optHoistLoopNest.

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 23, 2022

ghost assigned AndyAyersMS Jun 23, 2022

BruceForstall approved these changes Jun 23, 2022

View reviewed changes

AndyAyersMS added 2 commits June 23, 2022 13:52

opt more phases into post phase checks

6cd5761

even more phases

09d47cc

AndyAyersMS added 3 commits June 23, 2022 19:47

add hoisting, placeholders for missing phases

0b9f8b5

add comment showing when phase checks turn on

ffe8911

fix typo

9a58468

fix printf arg

6002912

BruceForstall requested changes Jun 27, 2022

View reviewed changes

ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 27, 2022

ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 28, 2022

revise per feedback

a6f7a8a

BruceForstall approved these changes Jun 28, 2022

View reviewed changes

AndyAyersMS merged commit cd88b84 into dotnet:main Jun 28, 2022

kunalspathak reviewed Jun 29, 2022

View reviewed changes

JulieLeeMSFT mentioned this pull request Jul 28, 2022

What's new in .NET 7 Preview 7 [WIP] dotnet/core#7455

Closed

ghost locked as resolved and limited conversation to collaborators Jul 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: strengthen checking of the loop table #71184

JIT: strengthen checking of the loop table #71184

AndyAyersMS commented Jun 23, 2022

ghost commented Jun 23, 2022

AndyAyersMS commented Jun 23, 2022

BruceForstall left a comment

BruceForstall Jun 23, 2022

AndyAyersMS Jun 23, 2022

AndyAyersMS commented Jun 23, 2022

AndyAyersMS commented Jun 25, 2022

azure-pipelines bot commented Jun 25, 2022

AndyAyersMS commented Jun 27, 2022

AndyAyersMS commented Jun 27, 2022

BruceForstall left a comment

BruceForstall Jun 27, 2022

AndyAyersMS Jun 28, 2022

BruceForstall Jun 27, 2022

AndyAyersMS Jun 28, 2022

BruceForstall Jun 27, 2022

BruceForstall Jun 27, 2022

BruceForstall Jun 27, 2022

AndyAyersMS commented Jun 28, 2022

AndyAyersMS commented Jun 28, 2022

kunalspathak Jun 29, 2022

kunalspathak Jun 29, 2022

AndyAyersMS Jun 29, 2022

kunalspathak Jun 29, 2022

AndyAyersMS Jun 29, 2022

	if (!BasicBlock::sameTryRegion(optLoopTable[lnum].lpHead, treeBb))
	{
	JITDUMP(" ... not hoisting in " FMT_LP ", eh region constraint (pre-header try index %d, candidate " FMT_BB
	" try index %d\n",
	lnum, optLoopTable[lnum].lpHead->bbTryIndex, treeBb->bbNum, treeBb->bbTryIndex);
	return;
	}

JIT: strengthen checking of the loop table #71184

JIT: strengthen checking of the loop table #71184

Conversation

AndyAyersMS commented Jun 23, 2022

ghost commented Jun 23, 2022

AndyAyersMS commented Jun 23, 2022

BruceForstall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyAyersMS commented Jun 23, 2022

AndyAyersMS commented Jun 25, 2022

azure-pipelines bot commented Jun 25, 2022

AndyAyersMS commented Jun 27, 2022

AndyAyersMS commented Jun 27, 2022

BruceForstall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyAyersMS commented Jun 28, 2022

AndyAyersMS commented Jun 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment