[GVN] Excluding dead blocks before full redundancy eliminations #88556
Conversation
Dead blocks will never contribute to non-local load value definitions; keeping it will trigger one of the assertions when we're building SSA form for candidate loads during full redundancy eliminations. This fixes llvm#88051
|
@llvm/pr-subscribers-llvm-transforms Author: Min-Yih Hsu (mshockwave) ChangesDead blocks will never contribute to non-local load value definitions; keeping it will trigger one of the assertions when we're building SSA form for candidate loads during full redundancy eliminations. This fixes #88051 Full diff: https://github.com/llvm/llvm-project/pull/88556.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
index 67fb2a5da3bb71..d5e84bf02a13e1 100644
--- a/llvm/lib/Transforms/Scalar/GVN.cpp
+++ b/llvm/lib/Transforms/Scalar/GVN.cpp
@@ -1888,6 +1888,17 @@ bool GVNPass::processNonLocalLoad(LoadInst *Load) {
// load, then it is fully redundant and we can use PHI insertion to compute
// its value. Insert PHIs and remove the fully redundant value now.
if (UnavailableBlocks.empty()) {
+ // Excluding dead blocks, whose available values are undef. Note that we
+ // can't do this removal for partial redundancy.
+ auto AVEndIt = ValuesPerBlock.end();
+ auto AVBeginIt =
+ llvm::remove_if(ValuesPerBlock, [](const auto &AV) -> bool {
+ return AV.AV.isUndefValue();
+ });
+ ValuesPerBlock.erase(AVBeginIt, AVEndIt);
+ if (ValuesPerBlock.empty())
+ return Changed;
+
LLVM_DEBUG(dbgs() << "GVN REMOVING NONLOCAL LOAD: " << *Load << '\n');
// Perform PHI construction.
diff --git a/llvm/test/Transforms/GVN/pr88051.ll b/llvm/test/Transforms/GVN/pr88051.ll
new file mode 100644
index 00000000000000..c50545764097ac
--- /dev/null
+++ b/llvm/test/Transforms/GVN/pr88051.ll
@@ -0,0 +1,91 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt -S --passes='gvn' < %s | FileCheck %s
+
+@g = external global i32
+@e = external global [1 x i32]
+
+define signext i32 @main() nounwind {
+; CHECK-LABEL: define signext i32 @main(
+; CHECK-SAME: ) #[[ATTR0:[0-9]+]] {
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br label [[WHILE_BODY:%.*]]
+; CHECK: while.body:
+; CHECK-NEXT: [[DOTPRE15:%.*]] = phi i32 [ [[DOTPRE16:%.*]], [[FOR_END54:%.*]] ], [ undef, [[ENTRY:%.*]] ]
+; CHECK-NEXT: [[DOTPRE2:%.*]] = phi i32 [ [[DOTPRE3:%.*]], [[FOR_END54]] ], [ undef, [[ENTRY]] ]
+; CHECK-NEXT: [[TMP0:%.*]] = phi i32 [ [[TMP3:%.*]], [[FOR_END54]] ], [ undef, [[ENTRY]] ]
+; CHECK-NEXT: [[TMP1:%.*]] = phi i32 [ [[TMP4:%.*]], [[FOR_END54]] ], [ undef, [[ENTRY]] ]
+; CHECK-NEXT: br label [[FOR_BODY3:%.*]]
+; CHECK: for.body3:
+; CHECK-NEXT: br i1 false, label [[IF_ELSE:%.*]], label [[FOR_COND6_PREHEADER:%.*]]
+; CHECK: for.cond6.preheader:
+; CHECK-NEXT: br label [[IF_END49:%.*]]
+; CHECK: if.else:
+; CHECK-NEXT: [[TMP2:%.*]] = load ptr, ptr null, align 8
+; CHECK-NEXT: store volatile i32 0, ptr [[TMP2]], align 4
+; CHECK-NEXT: br i1 false, label [[IF_END:%.*]], label [[FOR_END54SPLITSPLIT:%.*]]
+; CHECK: if.end:
+; CHECK-NEXT: br i1 false, label [[IF_END_IF_END49_CRIT_EDGE:%.*]], label [[IF_END_FOR_END54_CRIT_EDGE:%.*]]
+; CHECK: if.end.if.end49_crit_edge:
+; CHECK-NEXT: br label [[IF_END49]]
+; CHECK: if.end.for.end54_crit_edge:
+; CHECK-NEXT: [[DOTPRE9:%.*]] = sext i32 [[TMP1]] to i64
+; CHECK-NEXT: br label [[FOR_END54]]
+; CHECK: if.end49:
+; CHECK-NEXT: br i1 true, label [[FOR_BODY3]], label [[IF_END49_FOR_END54SPLIT_CRIT_EDGE:%.*]]
+; CHECK: if.end49.for.end54split_crit_edge:
+; CHECK-NEXT: [[DOTPRE_PRE:%.*]] = load i32, ptr @g, align 4
+; CHECK-NEXT: [[DOTPRE8:%.*]] = sext i32 [[DOTPRE_PRE]] to i64
+; CHECK-NEXT: br label [[FOR_END54SPLIT:%.*]]
+; CHECK: for.end54splitsplit:
+; CHECK-NEXT: [[IDXPROM55_PHI_TRANS_INSERT_PHI_TRANS_INSERT:%.*]] = sext i32 [[DOTPRE2]] to i64
+; CHECK-NEXT: [[ARRAYIDX56_PHI_TRANS_INSERT_PHI_TRANS_INSERT:%.*]] = getelementptr inbounds [1 x i32], ptr @e, i64 0, i64 [[IDXPROM55_PHI_TRANS_INSERT_PHI_TRANS_INSERT]]
+; CHECK-NEXT: [[DOTPRE1_PRE:%.*]] = load i32, ptr [[ARRAYIDX56_PHI_TRANS_INSERT_PHI_TRANS_INSERT]], align 4
+; CHECK-NEXT: br label [[FOR_END54SPLIT]]
+; CHECK: for.end54split:
+; CHECK-NEXT: [[IDXPROM55_PHI_TRANS_INSERT_PRE_PHI:%.*]] = phi i64 [ [[IDXPROM55_PHI_TRANS_INSERT_PHI_TRANS_INSERT]], [[FOR_END54SPLITSPLIT]] ], [ [[DOTPRE8]], [[IF_END49_FOR_END54SPLIT_CRIT_EDGE]] ]
+; CHECK-NEXT: [[DOTPRE1:%.*]] = phi i32 [ [[DOTPRE1_PRE]], [[FOR_END54SPLITSPLIT]] ], [ [[DOTPRE15]], [[IF_END49_FOR_END54SPLIT_CRIT_EDGE]] ]
+; CHECK-NEXT: [[DOTPRE:%.*]] = phi i32 [ [[DOTPRE2]], [[FOR_END54SPLITSPLIT]] ], [ [[DOTPRE_PRE]], [[IF_END49_FOR_END54SPLIT_CRIT_EDGE]] ]
+; CHECK-NEXT: [[ARRAYIDX56_PHI_TRANS_INSERT:%.*]] = getelementptr inbounds [1 x i32], ptr @e, i64 0, i64 [[IDXPROM55_PHI_TRANS_INSERT_PRE_PHI]]
+; CHECK-NEXT: br label [[FOR_END54]]
+; CHECK: for.end54:
+; CHECK-NEXT: [[IDXPROM55_PRE_PHI:%.*]] = phi i64 [ [[IDXPROM55_PHI_TRANS_INSERT_PRE_PHI]], [[FOR_END54SPLIT]] ], [ [[DOTPRE9]], [[IF_END_FOR_END54_CRIT_EDGE]] ]
+; CHECK-NEXT: [[DOTPRE16]] = phi i32 [ [[DOTPRE1]], [[FOR_END54SPLIT]] ], [ [[DOTPRE15]], [[IF_END_FOR_END54_CRIT_EDGE]] ]
+; CHECK-NEXT: [[DOTPRE3]] = phi i32 [ [[DOTPRE]], [[FOR_END54SPLIT]] ], [ [[DOTPRE2]], [[IF_END_FOR_END54_CRIT_EDGE]] ]
+; CHECK-NEXT: [[TMP3]] = phi i32 [ [[DOTPRE1]], [[FOR_END54SPLIT]] ], [ [[TMP0]], [[IF_END_FOR_END54_CRIT_EDGE]] ]
+; CHECK-NEXT: [[TMP4]] = phi i32 [ [[DOTPRE]], [[FOR_END54SPLIT]] ], [ [[TMP1]], [[IF_END_FOR_END54_CRIT_EDGE]] ]
+; CHECK-NEXT: [[ARRAYIDX56:%.*]] = getelementptr inbounds [1 x i32], ptr @e, i64 0, i64 [[IDXPROM55_PRE_PHI]]
+; CHECK-NEXT: tail call void null(i32 noundef signext [[TMP3]])
+; CHECK-NEXT: br label [[WHILE_BODY]]
+;
+entry:
+ br label %while.body
+
+while.body: ; preds = %for.end54, %entry
+ br label %for.body3
+
+for.body3: ; preds = %if.end49, %while.body
+ br i1 false, label %if.else, label %for.cond6.preheader
+
+for.cond6.preheader: ; preds = %for.body3
+ br label %if.end49
+
+if.else: ; preds = %for.body3
+ %0 = load ptr, ptr null, align 8
+ store volatile i32 0, ptr %0, align 4
+ br i1 false, label %if.end, label %for.end54
+
+if.end: ; preds = %if.else
+ br i1 false, label %if.end49, label %for.end54
+
+if.end49: ; preds = %if.end, %for.cond6.preheader
+ br i1 true, label %for.body3, label %for.end54
+
+for.end54: ; preds = %if.end49, %if.end, %if.else
+ %1 = load i32, ptr @g, align 4
+ %idxprom55 = sext i32 %1 to i64
+ %arrayidx56 = getelementptr inbounds [1 x i32], ptr @e, i64 0, i64 %idxprom55
+ %2 = load i32, ptr %arrayidx56, align 4
+ tail call void null(i32 noundef signext %2)
+ br label %while.body
+}
+
|
|
To give a little more context on the root cause of #88051, here is the CFG from the reduced test case: GVN crashed when it's processing the This patch filters out any dead blocks from the list of candidate values right before full redundancy elimination. |
|
Would it be possible / make sense to remove the assertion instead? As I understand it, doing a replacement with undef should be correct here, right? Looking at the commit that introduced the assertion (3168ab3) I don't think think that the fix being applied here is right. It sounds like maybe DeadBlocks is out of sync? |
Let me give it a try.
I think by saying out of sync you're referring to the on-the-fly edge splitting mentioned in 3168ab3, which I don't think it happen in this case. The |
Dead blocks will never contribute to non-local load value definitions; keeping it will trigger one of the assertions when we're building SSA form for candidate loads during full redundancy eliminations.
This fixes #88051