[SCEV] Fix exit condition for recursive loop guard collection #120442

juliannagele · 2024-12-18T15:43:59Z

When assumptions are present Terms.size() does not actually count the number of conditions collected from dominating branches; introduce a separate counter.

Fixes #120237

When assumptions are present Terms.size() does not actually count the number of conditions collected from dominating branches; introduce a separate counter.

llvmbot · 2024-12-18T15:44:41Z

@llvm/pr-subscribers-llvm-analysis

Author: Julian Nagele (juliannagele)

Changes

When assumptions are present Terms.size() does not actually count the number of conditions collected from dominating branches; introduce a separate counter.

Full diff: https://github.com/llvm/llvm-project/pull/120442.diff

1 Files Affected:

(modified) llvm/lib/Analysis/ScalarEvolution.cpp (+2-1)

diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp
index d55d09020fc147..6ba91c4417f724 100644
--- a/llvm/lib/Analysis/ScalarEvolution.cpp
+++ b/llvm/lib/Analysis/ScalarEvolution.cpp
@@ -15753,6 +15753,7 @@ void ScalarEvolution::LoopGuards::collectFromBlock(
   // predecessors that can be found that have unique successors leading to the
   // original header.
   // TODO: share this logic with isLoopEntryGuardedByCond.
+  unsigned NumCollectedConditions = 0;
   std::pair<const BasicBlock *, const BasicBlock *> Pair(Pred, Block);
   for (; Pair.first;
        Pair = SE.getPredecessorWithUniqueSuccessorForBB(Pair.first)) {
@@ -15767,7 +15768,7 @@ void ScalarEvolution::LoopGuards::collectFromBlock(
 
     // If we are recursively collecting guards stop after 2
     // predecessors to limit compile-time impact for now.
-    if (Depth > 0 && Terms.size() == 2)
+    if (Depth > 0 && ++NumCollectedConditions == 2)
       break;
   }
   // Finally, if we stopped climbing the predecessor chain because

fhahn · 2024-12-18T15:46:17Z

Could you add a test based on the reproducer from the issue?

Add test

juliannagele · 2024-12-18T16:15:00Z

Could you add a test based on the reproducer from the issue?

Ah good point, added!

fhahn · 2024-12-19T16:22:24Z

llvm/test/Analysis/ScalarEvolution/pr120442.ll

+bb1:
+  br label %bb2
+
+bb2:


Suggested change

bb2:

inner.header:

Might be good to reorder and rename the blocks to make the sturcture easier to see in the test

Thanks, simplified the test a bit more

fhahn · 2024-12-19T16:23:09Z

llvm/test/Analysis/ScalarEvolution/pr120442.ll

+bb5:
+  br i1 false, label %bb6, label %bb5
+
+bb6:


Suggested change

bb6:

outer.header:

fhahn · 2024-12-19T16:29:29Z

llvm/lib/Analysis/ScalarEvolution.cpp

@@ -15767,7 +15768,7 @@ void ScalarEvolution::LoopGuards::collectFromBlock(

    // If we are recursively collecting guards stop after 2
    // predecessors to limit compile-time impact for now.
-    if (Depth > 0 && Terms.size() == 2)
+    if (Depth > 0 && ++NumCollectedConditions == 2)


Ah I see, the issue is when we have cases where Terms already has entries from assumes.

Suggested change

if (Depth > 0 && ++NumCollectedConditions == 2)

++ NumPredecessors;

if (Depth > 0 && NumVisitedPredecessors == 2)

This aligns better with the comment immediately above. Might also be worth pulling out the increment of the condition to simplify for the reader

Hm, I think it is actually counting the number of conditions, due to the early continue above, changed the comment instead (and pulled out the increment) :)

fhahn · 2024-12-19T16:30:22Z

llvm/test/Analysis/ScalarEvolution/pr120442.ll

+  call void @llvm.assume(i1 false)
+  call void @llvm.assume(i1 false)


Suggested change

call void @llvm.assume(i1 false)

call void @llvm.assume(i1 false)

call void @llvm.assume(i1 %c.1)

call void @llvm.assume(i1 %c.2)

to avoid having immediate UB

fhahn · 2024-12-19T16:30:45Z

llvm/test/Analysis/ScalarEvolution/pr120442.ll

+
+declare void @llvm.assume(i1)
+
+define void @pr120442() {


Can you add a brief comment explaining what the test checks?

Thanks, added a comment

…ection

nikic

LG

nikic · 2024-12-20T11:13:41Z

llvm/test/Analysis/ScalarEvolution/pr120442.ll

Can you please add this test to the same file as the other tests for this code in backedge-taken-count-guard-info-with-multiple-predecessors.ll?

fhahn

LGTM, thanks!

Could you add

Fixes #120237

to the PR description to make sure the issue will get automatically closed when this lands?

Please also check if #120615 is also fixed by this

…rd collection

…20442) When assumptions are present `Terms.size()` does not actually count the number of conditions collected from dominating branches; introduce a separate counter. Fixes llvm#120237 (cherry picked from commit acfd26a)

…9797) * [SCEV] Fix exit condition for recursive loop guard collection (llvm#120442) When assumptions are present `Terms.size()` does not actually count the number of conditions collected from dominating branches; introduce a separate counter. Fixes llvm#120237 (cherry picked from commit acfd26a) * [SCEV] Make sure starting block is marked as visited when recursively collecting loop guards. (llvm#120749) When `collectFromBlock` is called without a predecessor (in particular for loops that don't have a unique predecessor outside the loop) we never start climbing the predecessor chain, and thus don't mark the starting block as visited. Fixes llvm#120615. (cherry picked from commit f035351)

[SCEV] Fix exit condition for recursive loop guard collection

057a47a

When assumptions are present Terms.size() does not actually count the number of conditions collected from dominating branches; introduce a separate counter.

juliannagele requested a review from fhahn December 18, 2024 15:44

juliannagele requested a review from nikic as a code owner December 18, 2024 15:44

juliannagele linked an issue Dec 18, 2024 that may be closed by this pull request

Program terminated with signal: SIGKILL Compiler returned: 137 #120237

Closed

llvmbot added the llvm:analysis Includes value tracking, cost tables and constant folding label Dec 18, 2024

juliannagele mentioned this pull request Dec 18, 2024

Program terminated with signal: SIGKILL Compiler returned: 137 #120237

Closed

fixup! [SCEV] Fix exit condition for recursive loop guard collection

a276fdf

Add test

fhahn reviewed Dec 19, 2024

View reviewed changes

fixup! fixup! [SCEV] Fix exit condition for recursive loop guard coll…

a2b7e86

…ection

nikic approved these changes Dec 20, 2024

View reviewed changes

fhahn reviewed Dec 20, 2024

View reviewed changes

fhahn approved these changes Dec 20, 2024

View reviewed changes

fixup! fixup! fixup! [SCEV] Fix exit condition for recursive loop gua…

5e574d4

…rd collection

juliannagele merged commit acfd26a into llvm:main Dec 20, 2024
8 checks passed

juliannagele mentioned this pull request Jan 6, 2025

[cherrypick][SCEV] Pick fixes for recursive loop guard collection. swiftlang/llvm-project#9797

Merged

	if (Depth > 0 && ++NumCollectedConditions == 2)
	++ NumPredecessors;
	if (Depth > 0 && NumVisitedPredecessors == 2)

		call void @llvm.assume(i1 false)
		call void @llvm.assume(i1 false)

[SCEV] Fix exit condition for recursive loop guard collection #120442

[SCEV] Fix exit condition for recursive loop guard collection #120442

Uh oh!

Conversation

juliannagele commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 18, 2024

Uh oh!

fhahn commented Dec 18, 2024

Uh oh!

juliannagele commented Dec 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

juliannagele commented Dec 18, 2024 •

edited

Loading