[DFAJumpThreading] Try harder to avoid cycles in paths. #169151
Merged
+223
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Change-Id: I410868c905e5d88a9ac3cbe6b8a829947377cafd
If a threading path has cycles within it then the transfomation is not correct. This patch fixes a couple of cases that create such cycles. Fixes llvm#166868 Change-Id: I8aeb7bc5e25c7b9565c60da937aea14c58d74e3e
Member
|
@llvm/pr-subscribers-llvm-transforms Author: Usman Nadeem (UsmanNadeem) ChangesIf a threading path has cycles within it then the transformation is not correct. This patch fixes a couple of cases that create such cycles. Fixes #166868 Full diff: https://github.com/llvm/llvm-project/pull/169151.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp b/llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp
index e84ca819b93d8..2f1f59c1ff2a8 100644
--- a/llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp
+++ b/llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp
@@ -625,8 +625,12 @@ struct AllSwitchPaths {
NewPath.setDeterminator(PhiBB);
NewPath.setExitValue(C);
// Don't add SwitchBlock at the start, this is handled later.
- if (IncomingBB != SwitchBlock)
+ if (IncomingBB != SwitchBlock) {
+ // Don't add a cycle to the path.
+ if (VB.contains(IncomingBB))
+ continue;
NewPath.push_back(IncomingBB);
+ }
NewPath.push_back(PhiBB);
Res.push_back(NewPath);
continue;
@@ -815,7 +819,12 @@ struct AllSwitchPaths {
std::vector<ThreadingPath> TempList;
for (const ThreadingPath &Path : PathsToPhiDef) {
+ SmallPtrSet<BasicBlock *, 32> PathSet(Path.getPath().begin(),
+ Path.getPath().end());
for (const PathType &PathToSw : PathsToSwitchBB) {
+ if (any_of(llvm::drop_begin(PathToSw),
+ [&](const BasicBlock *BB) { return PathSet.contains(BB); }))
+ continue;
ThreadingPath PathCopy(Path);
PathCopy.appendExcludingFirst(PathToSw);
TempList.push_back(PathCopy);
diff --git a/llvm/test/Transforms/DFAJumpThreading/dfa-jump-threading-analysis.ll b/llvm/test/Transforms/DFAJumpThreading/dfa-jump-threading-analysis.ll
index 5076517d92c74..7b8794ec22af5 100644
--- a/llvm/test/Transforms/DFAJumpThreading/dfa-jump-threading-analysis.ll
+++ b/llvm/test/Transforms/DFAJumpThreading/dfa-jump-threading-analysis.ll
@@ -246,5 +246,82 @@ exit:
ret i32 0
}
+define i8 @cyclesInPaths1(i1 %call12, i1 %cmp18) {
+; CHECK-LABEL: DFA Jump threading: cyclesInPaths1
+; CHECK-NOT: <{{.*}}> [{{.*}}]
+
+entry:
+ br label %switchPhiDef.for.body
+
+switchPhiDef.for.body: ; preds = %detBB2, %entry
+ %switchPhi.curState = phi i32 [ %curState.1, %detBB2 ], [ 0, %entry ]
+ br i1 %call12, label %detBB1, label %if.else
+
+if.else: ; preds = %switchPhiDef.for.body
+ br label %detBB1
+
+detBB1: ; preds = %if.else, %switchPhiDef.for.body
+ %newState.02 = phi i32 [ 2, %switchPhiDef.for.body ], [ 4, %if.else ]
+ br i1 %cmp18, label %detBB2, label %if.end20
+
+if.end20: ; preds = %detBB1
+ br i1 %call12, label %if.end23, label %switchBB
+
+switchBB: ; preds = %if.end20
+ switch i32 %switchPhi.curState, label %if.end23 [
+ i32 4, label %ret1
+ i32 0, label %ret2
+ ]
+
+ret1: ; preds = %switchBB
+ ret i8 1
+
+ret2: ; preds = %switchBB
+ ret i8 2
+
+if.end23: ; preds = %switchBB, %if.end20
+ br label %detBB2
+
+detBB2: ; preds = %if.end23, %detBB1
+ %curState.1 = phi i32 [ %newState.02, %if.end23 ], [ 0, %detBB1 ]
+ br label %switchPhiDef.for.body
+}
+
+define void @cyclesInPaths2(i1 %tobool5.not.i, ptr %P.sroa.0.050) {
+; CHECK-LABEL: DFA Jump threading: cyclesInPaths2
+; CHECK: < sw.bb.i, bb.exit, if.end5, if.end.i > [ 1, bb.exit ]
+; CHECK-NEXT: < bb.exit, if.end5, if.end.i > [ 0, bb.exit ]
+
+entry:
+ br label %if.end5
+
+if.end5: ; preds = %bb.exit, %entry
+ %P.sroa.8.051 = phi i16 [ %retval.sroa.6.0.i, %bb.exit ], [ 0, %entry ]
+ call void @foo3()
+ br i1 %tobool5.not.i, label %if.end.i, label %bb.exit
+
+if.end.i: ; preds = %if.end5
+ switch i16 %P.sroa.8.051, label %sw.default.i [
+ i16 1, label %sw.bb.i
+ i16 0, label %bb.exit
+ ]
+
+sw.bb.i: ; preds = %if.end.i
+ call void @foo1()
+ br label %bb.exit
+
+sw.default.i: ; preds = %if.end.i
+ unreachable
+
+bb.exit: ; preds = %sw.bb.i, %if.end.i, %if.end5
+ %retval.sroa.6.0.i = phi i16 [ 1, %sw.bb.i ], [ 0, %if.end5 ], [ 0, %if.end.i ]
+ call void @foo2()
+ br label %if.end5
+}
+
+declare void @foo1()
+declare void @foo2()
+declare void @foo3()
+
!0 = !{!"function_entry_count", i32 10}
!1 = !{!"branch_weights", i32 3, i32 5}
|
XChy
reviewed
Nov 22, 2025
Comment on lines
+825
to
+827
| if (any_of(llvm::drop_begin(PathToSw), | ||
| [&](const BasicBlock *BB) { return PathSet.contains(BB); })) | ||
| continue; |
Member
There was a problem hiding this comment.
Is it necessary? IIUC, VB should record all the visited blocks before calculating PathsToSwitchBB, and paths don't consider these visited blocks again.
Member
There was a problem hiding this comment.
Yep, I understand it now when debugging. We actually don't guarantee that there are no common blocks between PathsToPhiDef and PathsToSwitchBB by checking VB, as we have popped out all inserted elements in VB after paths call.
XChy
reviewed
Nov 22, 2025
| ret i32 0 | ||
| } | ||
|
|
||
| define i8 @cyclesInPaths1(i1 %call12, i1 %cmp18) { |
Member
There was a problem hiding this comment.
Could you add similar tests to show the final transformed IR?
XChy
approved these changes
Nov 22, 2025
Change-Id: I23a291c3ef2ab221065aa5759ea13a5d2cfc20bf
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/14180 Here is the relevant piece of the build log for the reference |
aadeshps-mcw
pushed a commit
to aadeshps-mcw/llvm-project
that referenced
this pull request
Nov 26, 2025
If a threading path has cycles within it then the transformation is not correct. This patch fixes a couple of cases that create such cycles. Fixes llvm#166868
Priyanshu3820
pushed a commit
to Priyanshu3820/llvm-project
that referenced
this pull request
Nov 26, 2025
If a threading path has cycles within it then the transformation is not correct. This patch fixes a couple of cases that create such cycles. Fixes llvm#166868
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If a threading path has cycles within it then the transformation is not correct. This patch fixes a couple of cases that create such cycles.
Fixes #166868