Skip to content

[IR][JumpThreading] Fix infinite recursion on compare self-reference [updated] #129501

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 22 commits into from
Apr 7, 2025
Merged

[IR][JumpThreading] Fix infinite recursion on compare self-reference [updated] #129501

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
b1b2f0a
[IR] Avoid self-referencing values caused by PHI node removal
ro-i Mar 2, 2025
c97a6c6
implement feedback
ro-i Mar 3, 2025
d9e764f
implement another approach
ro-i Mar 5, 2025
e0ccb61
replace undef
ro-i Mar 5, 2025
35aa07d
implement another approach; reset existing tests
ro-i Mar 7, 2025
573c2a9
fix formatting
ro-i Mar 7, 2025
24a1fca
more precise naming
ro-i Mar 7, 2025
f2355f5
Revert "more precise naming"
ro-i Mar 21, 2025
452e7c0
Revert "fix formatting"
ro-i Mar 21, 2025
e35945f
Revert "implement another approach; reset existing tests"
ro-i Mar 21, 2025
2a419cb
Revert "replace undef"
ro-i Mar 21, 2025
e0b9786
Revert "implement another approach"
ro-i Mar 21, 2025
13e3598
Revert "implement feedback"
ro-i Mar 21, 2025
9fe2146
Revert "[IR] Avoid self-referencing values caused by PHI node removal"
ro-i Mar 21, 2025
a488438
[IR][JumpThreading] Fix infinite recursion on compare self-reference
ro-i Mar 21, 2025
548f359
fix formatting
ro-i Mar 21, 2025
03ae290
implement feedback
ro-i Mar 21, 2025
60f2856
public -> private
ro-i Mar 24, 2025
de6ae29
implemented feedback
ro-i Mar 25, 2025
b611801
implement feedback
ro-i Mar 27, 2025
643cb86
insert and double check at function start
ro-i Apr 5, 2025
1ae1449
use make_scope_exit
ro-i Apr 6, 2025
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
16 changes: 12 additions & 4 deletions llvm/lib/Transforms/Scalar/JumpThreading.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1519,12 +1519,20 @@ Constant *JumpThreadingPass::evaluateOnPredecessorEdge(BasicBlock *BB,
}

// If we have a CmpInst, try to fold it for each incoming edge into PredBB.
// Note that during the execution of the pass, phi nodes may become constant
// and may be removed, which can lead to self-referencing instructions in
// code that becomes unreachable. Consequently, we need to handle those
// instructions in unreachable code and check before going into recursion.
if (CmpInst *CondCmp = dyn_cast<CmpInst>(V)) {
if (CondCmp->getParent() == BB) {
Constant *Op0 =
evaluateOnPredecessorEdge(BB, PredPredBB, CondCmp->getOperand(0), DL);
Constant *Op1 =
evaluateOnPredecessorEdge(BB, PredPredBB, CondCmp->getOperand(1), DL);
Constant *Op0 = CondCmp->getOperand(0) == CondCmp
Copy link
Contributor

@nikic nikic Mar 21, 2025

Choose a reason for hiding this comment

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

This handles one specific kind of recursion, but there may be recursion across multiple instructions. It would be better to add a Visited set instead.

Copy link
Contributor Author

@ro-i ro-i Mar 21, 2025

Choose a reason for hiding this comment

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

You mean directly in evaluateOnPredecessorEdge?

Copy link
Contributor Author

@ro-i ro-i Mar 21, 2025

Choose a reason for hiding this comment

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

We could do something like this (see the commit I just pushed), I guess

Copy link
Contributor Author

@ro-i ro-i Mar 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

(+ I guess this could/should be a private method)

? nullptr
: evaluateOnPredecessorEdge(
BB, PredPredBB, CondCmp->getOperand(0), DL);
Constant *Op1 = CondCmp->getOperand(1) == CondCmp
? nullptr
: evaluateOnPredecessorEdge(
BB, PredPredBB, CondCmp->getOperand(1), DL);
if (Op0 && Op1) {
return ConstantFoldCompareInstOperands(CondCmp->getPredicate(), Op0,
Op1, DL);
Expand Down
59 changes: 59 additions & 0 deletions llvm/test/Transforms/JumpThreading/unreachable-loops.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,4 +180,63 @@ cleanup2343.loopexit4: ; preds = %cleanup1491
unreachable
}

; This segfaults due to recursion in %C4. Reason: %L6 is identified to be a
; "partially redundant load" and is replaced by a PHI node. The PHI node is then
; simplified to be constant and is removed. This leads to %L6 being replaced by
; %C4, which makes %C4 invalid since it uses %L6.
; The test case has been generated by the AMD Fuzzing project and simplified
; manually and by llvm-reduce.

define i32 @constant_phi_leads_to_self_reference(ptr %ptr) {
; CHECK-LABEL: @constant_phi_leads_to_self_reference(
; CHECK-NEXT: [[A9:%.*]] = alloca i1, align 1
; CHECK-NEXT: br label [[F6:%.*]]
; CHECK: T3:
; CHECK-NEXT: br label [[BB5:%.*]]
; CHECK: BB5:
; CHECK-NEXT: [[L10:%.*]] = load i1, ptr [[A9]], align 1
; CHECK-NEXT: br i1 [[L10]], label [[BB6:%.*]], label [[F6]]
; CHECK: BB6:
; CHECK-NEXT: [[LGV3:%.*]] = load i1, ptr [[PTR:%.*]], align 1
; CHECK-NEXT: [[C4:%.*]] = icmp sle i1 [[C4]], true
; CHECK-NEXT: store i1 [[C4]], ptr [[PTR]], align 1
; CHECK-NEXT: br i1 [[C4]], label [[F6]], label [[T3:%.*]]
; CHECK: F6:
; CHECK-NEXT: ret i32 0
; CHECK: F7:
; CHECK-NEXT: br label [[BB5]]
;
%A9 = alloca i1, align 1
br i1 false, label %BB4, label %F6

BB4: ; preds = %0
br i1 false, label %F6, label %F1

F1: ; preds = %BB4
br i1 false, label %T4, label %T3

T3: ; preds = %T4, %BB6, %F1
%L6 = load i1, ptr %ptr, align 1
br label %BB5

BB5: ; preds = %F7, %T3
%L10 = load i1, ptr %A9, align 1
br i1 %L10, label %BB6, label %F6

BB6: ; preds = %BB5
%LGV3 = load i1, ptr %ptr, align 1
%C4 = icmp sle i1 %L6, true
store i1 %C4, ptr %ptr, align 1
br i1 %L6, label %F6, label %T3

T4: ; preds = %F1
br label %T3

F6: ; preds = %BB6, %BB5, %BB4, %0
ret i32 0

F7: ; No predecessors!
br label %BB5
}

!0 = !{!"branch_weights", i32 2146410443, i32 1073205}