[StructurizeCFG] Fix a crash caused by not updating Predicates properly
#124051
Conversation
…erly A very simple IR could crash the pass at `assert(BB != Parent);`. ``` define void @foo() { entry: br i1 false, label %cond.true, label %cond.false cond.true: ; preds = %entry br label %cond.end cond.false: ; preds = %entry br label %cond.end cond.end: ; preds = %cond.false, %cond.true ret void } ``` TBH I'm not sure if my fix is right. I don't know why we don't update `Predicates` after we replace a conditional branch with a branch, given the one not taken will no longer be a sucessor of the current BB. There are many test cases need to be updated but I'd like to update them after I fix this properly. Any help would be appreciated.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@llvm/pr-subscribers-backend-amdgpu Author: Shilei Tian (shiltian) ChangesA very simple IR could crash the pass at TBH I'm not sure if my fix is right. I don't know why we don't update Full diff: https://github.com/llvm/llvm-project/pull/124051.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/StructurizeCFG.cpp b/llvm/lib/Transforms/Scalar/StructurizeCFG.cpp
index b1f742b838f2a1..eaeb6ca4f3e04a 100644
--- a/llvm/lib/Transforms/Scalar/StructurizeCFG.cpp
+++ b/llvm/lib/Transforms/Scalar/StructurizeCFG.cpp
@@ -971,6 +971,10 @@ void StructurizeCFG::changeExit(RegionNode *Node, BasicBlock *NewExit,
SubRegion->replaceExit(NewExit);
} else {
BasicBlock *BB = Node->getNodeAs<BasicBlock>();
+ for (BasicBlock *Succ : successors(BB)) {
+ if (Succ != NewExit)
+ Predicates[Succ].erase(BB);
+ }
killTerminator(BB);
BranchInst *Br = BranchInst::Create(NewExit, BB);
Br->setDebugLoc(TermDL[BB]);
diff --git a/llvm/test/Transforms/StructurizeCFG/simple-structurizecfg-crash.ll b/llvm/test/Transforms/StructurizeCFG/simple-structurizecfg-crash.ll
new file mode 100644
index 00000000000000..15983c448588b3
--- /dev/null
+++ b/llvm/test/Transforms/StructurizeCFG/simple-structurizecfg-crash.ll
@@ -0,0 +1,26 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -passes=structurizecfg %s -o - | FileCheck %s
+
+define void @foo() {
+; CHECK-LABEL: define void @foo() {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: br label %[[COND_FALSE:.*]]
+; CHECK: [[COND_TRUE:.*]]:
+; CHECK-NEXT: br label %[[COND_END:.*]]
+; CHECK: [[COND_FALSE]]:
+; CHECK-NEXT: br i1 false, label %[[COND_TRUE]], label %[[COND_END]]
+; CHECK: [[COND_END]]:
+; CHECK-NEXT: ret void
+;
+entry:
+ br i1 false, label %cond.true, label %cond.false
+
+cond.true: ; preds = %entry
+ br label %cond.end
+
+cond.false: ; preds = %entry
+ br label %cond.end
+
+cond.end: ; preds = %cond.false, %cond.true
+ ret void
+}
|
| for (BasicBlock *Succ : successors(BB)) { | ||
| if (Succ != NewExit) | ||
| Predicates[Succ].erase(BB); | ||
| } |
There was a problem hiding this comment.
Probably should move this into killTerminator where the actual successor removal occurs
There was a problem hiding this comment.
does it mean that we indeed need to update Predicates here? I thought the no-update was on purpose.
There was a problem hiding this comment.
What makes you think it's on purpose?
The predicates collects branch conditions used in the CFG, so it makes sense to me that you would need to adjust those when the CFG changes
There was a problem hiding this comment.
Hmm, alright. It has been sitting there for 10+ years so my first reaction is, probably it was designed in that way...
|
This is not a correct fix and I gave it up. |
Can you elaborate on how it's not correct? At minimum should commit the xfailed test |
It looks like the predicate was not constructed correctly at the first place but I don't understand some logic behind how it is constructed after spending several days on it, so I gave it up. After the change made in this PR, a lot of tests need to be updated. I checked some of them. They don't look like the right change.
Sure, will do that. |
I tried to fix it in #124051 but failed to do so. This PR adds the test and marks it as xfail.
I tried to fix it in #124051 but failed to do so. This PR adds the test and marks it as xfail.
…lvm#126087) I tried to fix it in llvm#124051 but failed to do so. This PR adds the test and marks it as xfail.
…lvm#126087) I tried to fix it in llvm#124051 but failed to do so. This PR adds the test and marks it as xfail.
A very simple IR could crash the pass at
assert(BB != Parent);.TBH I'm not sure if my fix is right. I don't know why we don't update
Predicatesafter we replace a conditional branch with a branch, given the onenot taken will no longer be a sucessor of the current BB. There are many test
cases need to be updated but I'd like to update them after I fix this properly.
Any help would be appreciated.