Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 20, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
057a47a
[SCEV] Fix exit condition for recursive loop guard collection
juliannagele Dec 18, 2024
a276fdf
fixup! [SCEV] Fix exit condition for recursive loop guard collection
juliannagele Dec 18, 2024
a2b7e86
fixup! fixup! [SCEV] Fix exit condition for recursive loop guard coll…
juliannagele Dec 20, 2024
5e574d4
fixup! fixup! fixup! [SCEV] Fix exit condition for recursive loop gua…
juliannagele Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion llvm/lib/Analysis/ScalarEvolution.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand All @@ -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)
Copy link
Contributor

@fhahn fhahn Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@juliannagele juliannagele Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) :)

break;
}
// Finally, if we stopped climbing the predecessor chain because
Expand Down
35 changes: 35 additions & 0 deletions llvm/test/Analysis/ScalarEvolution/pr120442.ll
View file
Open in desktop
Copy link
Contributor

@nikic nikic Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@juliannagele juliannagele Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done

Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
; RUN: opt < %s -disable-output "-passes=print<scalar-evolution>" -scalar-evolution-max-iterations=0 -scalar-evolution-classify-expressions=0 2>&1 | FileCheck %s

declare void @llvm.assume(i1)

define void @pr120442() {
Copy link
Contributor

@fhahn fhahn Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@juliannagele juliannagele Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, added a comment

; CHECK-LABEL: 'pr120442'
; CHECK-NEXT: Determining loop execution counts for: @pr120442
; CHECK-NEXT: Loop %bb2: backedge-taken count is i32 0
; CHECK-NEXT: Loop %bb2: constant max backedge-taken count is i32 0
; CHECK-NEXT: Loop %bb2: symbolic max backedge-taken count is i32 0
; CHECK-NEXT: Loop %bb2: Trip multiple is 1
; CHECK-NEXT: Loop %bb1: <multiple exits> Unpredictable backedge-taken count.
; CHECK-NEXT: Loop %bb1: Unpredictable constant max backedge-taken count.
; CHECK-NEXT: Loop %bb1: Unpredictable symbolic max backedge-taken count.
bb:
call void @llvm.assume(i1 false)
call void @llvm.assume(i1 false)
Copy link
Contributor

@fhahn fhahn Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@juliannagele juliannagele Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

br label %bb6

bb1:
br label %bb2

bb2:
Copy link
Contributor

@fhahn fhahn Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bb2:
inner.header:

Copy link
Contributor

@fhahn fhahn Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@juliannagele juliannagele Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, simplified the test a bit more

%phi = phi i32 [ %add, %bb2 ], [ 0, %bb1 ]
%add = add i32 %phi, 1
%icmp = icmp ugt i32 %add, 0
br i1 %icmp, label %bb1, label %bb2

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

bb6:
Copy link
Contributor

@fhahn fhahn Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bb6:
outer.header:

%phi7 = phi i32 [ 0, %bb5 ], [ 0, %bb ]
br label %bb1
}
Loading