JIT: Validate loop invariant base case as part of IV analysis #97122

jakobbotsch · 2024-01-17T23:39:59Z

Move the validation of the base case of the IV analysis into FlowGraphNaturalLoop::AnalyzeIteration. Previously the validation for the base case was left up to the users:

Loop cloning tried to accomplish this by checking whether loop inversion had run, but this is not sufficient (JIT: Improper zero-trip checking in loop cloning/unrolling #96623) as nothing guarantees the loop is dominated by the inverted test. Loop cloning also skipped the check entirely for GDV, leading to JIT: Loop cloning when the method has GDV skips necessary validity checks #95315.
Unrolling completely neglected to check the condition leading to JIT: Loop unrolling does not validate predecessor BBJ_COND condition properly #96591. Furthermore, unrolling made implicit assumptions that any BBJ_COND init block was an inverted test and downright removed the condition without any checks. This also led to another issue Assertion failed 'loop->EntryEdge(0)->getSourceBlock()->KindIs(BBJ_ALWAYS)' #97040 where unrolling could uncover new natural loops that had not been canonicalized.

This change makes FlowGraphNaturalLoop::AnalyzeIteration try to prove that the loop invariant it returns is true in the base case as well as being maintained by the loop. If it cannot prove this then it fails. This fixes all the issues, but sadly uncovers that we were doing a lot of cloning in OSR methods without actually proving legality, and where it is actually illegal. We no longer clone in these cases, but we can look into what to do about them separately.

In addition, loop unrolling no longer removes the init block condition. We leave that up to RBO which is able to remove the trivial checks that loop inversion sometimes creates in generality. This does lead to a few diffs from removing the check later in the JIT pipeline.

Diffs expected from less cloning in OSR and from leaving the trivial inverted tests around until RBO.

Fix #95315
Fix #96591
Fix #96623
Fix #97040

Move the validation of the base case of the IV analysis into `FlowGraphNaturalLoop::AnalyzeIteration`. Previously the validation for the base case was left up to the users: - Loop cloning tried to accomplish this by checking whether loop inversion had run, but this is not sufficient (dotnet#96623) as nothing guarantees the loop is dominated by the inverted test. Loop cloning also skipped the check entirely for GDV, leading to dotnet#95315. - Unrolling completely neglected to check the condition leading to dotnet#96591. Furthermore, unrolling made implicit assumptions that any `BBJ_COND` init block was an inverted test and downright removed the condition without any checks. This also led to another issue dotnet#97040 where unrolling could uncover new natural loops that had not been canonicalized. This change makes `FlowGraphNaturalLoop::AnalyzeIteration` try to prove that the loop invariant it returns is true in the base case as well as being maintained by the loop. If it cannot prove this then it fails. This fixes all the issues, but sadly uncovers that we were doing a lot of cloning in OSR methods without actually proving legality. We no longer clone in these cases, but we can look into what to do about them separately. Fix dotnet#95315 Fix dotnet#96591 Fix dotnet#96623 Fix dotnet#97040

ghost · 2024-01-17T23:40:14Z

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

Issue Details

Move the validation of the base case of the IV analysis into FlowGraphNaturalLoop::AnalyzeIteration. Previously the validation for the base case was left up to the users:

Loop cloning tried to accomplish this by checking whether loop inversion had run, but this is not sufficient (JIT: Improper zero-trip checking in loop cloning/unrolling #96623) as nothing guarantees the loop is dominated by the inverted test. Loop cloning also skipped the check entirely for GDV, leading to JIT: Loop cloning when the method has GDV skips necessary validity checks #95315.
Unrolling completely neglected to check the condition leading to JIT: Loop unrolling does not validate predecessor BBJ_COND condition properly #96591. Furthermore, unrolling made implicit assumptions that any BBJ_COND init block was an inverted test and downright removed the condition without any checks. This also led to another issue Assertion failed 'loop->EntryEdge(0)->getSourceBlock()->KindIs(BBJ_ALWAYS)' #97040 where unrolling could uncover new natural loops that had not been canonicalized.

This change makes FlowGraphNaturalLoop::AnalyzeIteration try to prove that the loop invariant it returns is true in the base case as well as being maintained by the loop. If it cannot prove this then it fails. This fixes all the issues, but sadly uncovers that we were doing a lot of cloning in OSR methods without actually proving legality, and where it is actually illegal. We no longer clone in these cases, but we can look into what to do about them separately.

In addition, loop unrolling no longer removes the init block condition. We leave that up to RBO which is able to remove the trivial checks that loop inversion sometimes creates in generality. This does lead to a few diffs from removing the check later in the JIT pipeline.

Diffs expected from less cloning in OSR and from leaving the trivial inverted tests around until RBO.

Fix #95315
Fix #96591
Fix #96623
Fix #97040

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

jakobbotsch · 2024-01-17T23:43:11Z

src/coreclr/jit/flowgraph.cpp

+    {
+        GenTree* enteringJTrue = initBlock->lastStmt()->GetRootNode();
+        assert(enteringJTrue->OperIs(GT_JTRUE));
+        if (enteringJTrue->gtGetOp1()->OperIsCompare() && ((enteringJTrue->gtGetOp1()->gtFlags & GTF_RELOP_ZTT) != 0))


Note that loop cloning checks GTF_RELOP_ZTT that is set on the test tree within the loop, while this is checking for GTF_RELOP_ZTT set on the actual zero-trip test that we find through the init block. Loop inversion sets both, but checking the one inside the loop doesn't make sense because nothing validates that the test outside the loop actually dominates the loop. optExtractInitTestIncr does guarantee that the init block dominates the loop (as far as I can tell), which is why this fixes #96623.

I want to switch this to an IR based check and remove GTF_RELOP_ZTT so that we do not have this cross phase dependency on loop inversion. We'll be able to remove GTF_RELOP_ZTT when we do that. However, I'll do that separately.

jakobbotsch · 2024-01-17T23:46:19Z

src/coreclr/jit/flowgraph.cpp

@@ -5340,7 +5466,7 @@ var_types NaturalLoopIterInfo::IterOperType()
 //
 bool NaturalLoopIterInfo::IsReversed()
 {
-    return TestTree->gtGetOp2()->OperIs(GT_LCL_VAR) && ((TestTree->gtGetOp2()->gtFlags & GTF_VAR_ITERATOR) != 0);
+    return TestTree->gtGetOp2()->OperIsScalarLocal() && (TestTree->gtGetOp2()->AsLclVar()->GetLclNum() == IterVar);


We'll also be able to delete GTF_VAR_ITERATOR soon. Currently there is a use left in old loop finding.

jakobbotsch · 2024-01-18T10:27:17Z

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, Fuzzlyn

azure-pipelines · 2024-01-18T10:27:39Z

Azure Pipelines successfully started running 3 pipeline(s).

jakobbotsch · 2024-01-18T14:04:08Z

cc @dotnet/jit-contrib PTAL @BruceForstall

Diffs. As mentioned above primarily from no longer cloning in some OSR methods where we cannot prove it to be safe. Some small ones outside OSR methods from leaving trivial loop-inverted tests around until RBO instead of removing them in unrolling.

jakobbotsch · 2024-01-18T14:05:17Z

src/coreclr/jit/compiler.h

+    // Is the loop exited when TestTree is true?
+    bool ExitedOnTrue : 1;


Since I spent time understanding the logic I added this now, even though it is always expected to be false currently. I expect to make a future change that removes the use of GetLexicallyBottomMostBlock in AnalyzeIteration, at which point this will be able to be true.

BruceForstall · 2024-01-18T18:01:03Z

src/tests/JIT/Regression/JitBlue/Runtime_95315/Runtime_95315.cs

+    [Fact]
+    public static void TestEntryPoint()
+    {
+        for (int i = 0; i < 4; i++)


Why are the constants in this test chosen to be the values they are?

e.g., is "4" because the constant 4-count loop gets unrolled? 200 because it doesn't get unrolled?

It's to make sure we tier up completely (handling up to 4 potential tiers...)

BruceForstall · 2024-01-18T18:01:42Z

src/tests/JIT/Regression/JitBlue/Runtime_95315/Runtime_95315.cs

+
+        iface.Foo();
+
+        int i = 10000000;


why 10M? presumably it will fail after 10 iterations? (if it doesn't fail, we don't want the test writing 10MB of data)

I picked a large constant that has high probability to hit AV if we wrongly elide the bounds check. But you're right, we shouldn't be doing Console.WriteLine with it, will fix that

Note that arr.Length is only 1

On second thought, if we hit valid data at + 10000000 then we only print one time anyway, since i < arr.Length will immediately be false.

Going to leave it like this since it'll at most print one time.

BruceForstall · 2024-01-18T18:02:08Z

src/tests/JIT/Regression/JitBlue/Runtime_95315/Runtime_95315.cs

+                Assert.Throws<IndexOutOfRangeException>(() => Bar(new int[10], new C()));
+            }
+
+            Thread.Sleep(100);


This is to allow tiering up?

Yeah, this loop is just to tier up Bar. The problem is inside Bar

BruceForstall · 2024-01-18T18:03:27Z

src/coreclr/jit/compiler.h

@@ -2006,14 +2006,16 @@ struct NaturalLoopIterInfo
    // The local that is the induction variable.
    unsigned IterVar = BAD_VAR_NUM;

+#ifdef DEBUG
+    // Tree that initializes induction varaible outside the loop.


Suggested change

// Tree that initializes induction varaible outside the loop.

// Tree that initializes induction variable outside the loop.

Going to address this as part of a follow-up.

…#97122) Move the validation of the base case of the IV analysis into `FlowGraphNaturalLoop::AnalyzeIteration`. Previously the validation for the base case was left up to the users: - Loop cloning tried to accomplish this by checking whether loop inversion had run, but this is not sufficient (dotnet#96623) as nothing guarantees the loop is dominated by the inverted test. Loop cloning also skipped the check entirely for GDV, leading to dotnet#95315. - Unrolling completely neglected to check the condition leading to dotnet#96591. Furthermore, unrolling made implicit assumptions that any `BBJ_COND` init block was an inverted test and downright removed the condition without any checks. This also led to another issue dotnet#97040 where unrolling could uncover new natural loops that had not been canonicalized. This change makes `FlowGraphNaturalLoop::AnalyzeIteration` try to prove that the loop invariant it returns is true in the base case as well as being maintained by the loop. If it cannot prove this then it fails. This fixes all the issues, but sadly uncovers that we were doing a lot of cloning in OSR methods without actually proving legality. We no longer clone in these cases, but we can look into what to do about them separately. Fix dotnet#95315 Fix dotnet#96591 Fix dotnet#96623 Fix dotnet#97040

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

ghost assigned jakobbotsch Jan 17, 2024

jakobbotsch commented Jan 17, 2024

View reviewed changes

Run jit-format

fd5bbd6

This was referenced Jan 18, 2024

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

BasicEventSourceTests.TestsManifestGeneration test failing in CI #97103

Closed

build-analysis bot mentioned this pull request Jan 18, 2024

Assert failure: orObject->HasEmptySyncBlockInfo() #96790

Closed

jakobbotsch marked this pull request as ready for review January 18, 2024 14:03

jakobbotsch requested a review from BruceForstall January 18, 2024 14:04

jakobbotsch commented Jan 18, 2024

View reviewed changes

BruceForstall approved these changes Jan 18, 2024

View reviewed changes

jakobbotsch merged commit 2f63046 into dotnet:main Jan 18, 2024
202 of 206 checks passed

jakobbotsch deleted the fix-95315-96591 branch January 18, 2024 20:54

jakobbotsch mentioned this pull request Jan 18, 2024

JIT: parent loops of OSR transitioned loops are not cloned #97178

Open

This was referenced Jan 23, 2024

[Perf] Windows/x64: 3 Regressions on 1/18/2024 8:54:27 PM #97404

Open

[Perf] Linux/x64: 1 Regression on 1/18/2024 8:54:27 PM dotnet/perf-autofiling-issues#27721

Closed

github-actions bot locked and limited conversation to collaborators Feb 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Validate loop invariant base case as part of IV analysis #97122

JIT: Validate loop invariant base case as part of IV analysis #97122

jakobbotsch commented Jan 17, 2024

ghost commented Jan 17, 2024

jakobbotsch Jan 17, 2024 •

edited

Loading

jakobbotsch Jan 17, 2024

jakobbotsch commented Jan 18, 2024

azure-pipelines bot commented Jan 18, 2024

jakobbotsch commented Jan 18, 2024

jakobbotsch Jan 18, 2024

BruceForstall Jan 18, 2024

jakobbotsch Jan 18, 2024

BruceForstall Jan 18, 2024

jakobbotsch Jan 18, 2024 •

edited

Loading

jakobbotsch Jan 18, 2024

jakobbotsch Jan 18, 2024

jakobbotsch Jan 18, 2024

BruceForstall Jan 18, 2024

jakobbotsch Jan 18, 2024

BruceForstall Jan 18, 2024

jakobbotsch Jan 18, 2024

		// Is the loop exited when TestTree is true?
		bool ExitedOnTrue : 1;

	// Tree that initializes induction varaible outside the loop.
	// Tree that initializes induction variable outside the loop.

JIT: Validate loop invariant base case as part of IV analysis #97122

JIT: Validate loop invariant base case as part of IV analysis #97122

Conversation

jakobbotsch commented Jan 17, 2024

ghost commented Jan 17, 2024

jakobbotsch Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented Jan 18, 2024

azure-pipelines bot commented Jan 18, 2024

jakobbotsch commented Jan 18, 2024

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

jakobbotsch Jan 18, 2024 • edited Loading

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

jakobbotsch Jan 17, 2024 •

edited

Loading

jakobbotsch Jan 18, 2024 •

edited

Loading