[llvm] [NewGVN] Prevent cyclic dependencies by ensuring Leader has min RPO number (PR #82110)
via llvm-commits
llvm-commits at lists.llvm.org
Sat Feb 17 06:23:23 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-llvm-transforms
Author: None (ManuelJBrito)
<details>
<summary>Changes</summary>
Cyclic dependencies in NewGVN can result in miscompilation and termination issues.
Example:
```
define void @<!-- -->main(i1 %c1, i1 %c2, i32 %x) {
entry:
br i1 %c1, label %L, label %end
L:
%d.1 = phi i8 [ undef, %entry ], [ -1, %L ]
%conv = sext i8 %d.1 to i32
%xor = xor i32 %x, %conv
%neg = xor i32 %xor, -1
call void @<!-- -->foo(i32 %neg)
br label %L
end:
ret void
}
```
```
%d.1 = phi i8 [ undef, %entry ], [ -1, %L ] -> undef (ignores unreachable backedge)
%conv = sext i8 %d.1 to i32 -> 0 (refined by InstSimplify)
%xor = xor i32 %x, %conv -> %x
%neg = xor i32 %xor, -1 -> xor i32 %x, -1
edge (%L, %L) is marked as reachable => %d.1 is added to the worklist
%d.1 = phi i8 [ undef, %entry ], [ -1, %L ] -> -1 (ignores undef)
%conv = sext i8 %d.1 to i32 -> -1
%xor = xor i32 %x, %conv -> xor i32 %x, -1 (Leader is %neg)
%neg = xor i32 %xor, -1 -> xor i32 %x, -1 ( %xor is replaced by %neg => WRONG)
```
The problem is the cyclic dependence:
- %neg depends on %xor because it's an operand.
- %xor depends on %neg because it's the class leader.
This issue also leads to infinite evaluation loops, where NewGVN never converges due to the cyclic dependence.
This patch ensures that the Class Leader is always the member with the lowest RPO number. This ensures that the Class Leader is processed before all other members, making the cyclic dependence impossible.
This fixes #<!-- -->35683 and #<!-- -->69211.
Regressions:
- 'simp-to-self.ll' regresses due to a known limitation in the way NewGVN and InstSimplify interact. With the new leader, InstSimplify does not know that %conv is 1 and fails to simplify.
---
Full diff: https://github.com/llvm/llvm-project/pull/82110.diff
4 Files Affected:
- (modified) llvm/lib/Transforms/Scalar/NewGVN.cpp (+32-17)
- (renamed) llvm/test/Transforms/NewGVN/pr36335-phi-undef.ll (+2-3)
- (added) llvm/test/Transforms/NewGVN/pr69211.ll (+58)
- (modified) llvm/test/Transforms/NewGVN/simp-to-self.ll (+16-5)
``````````diff
diff --git a/llvm/lib/Transforms/Scalar/NewGVN.cpp b/llvm/lib/Transforms/Scalar/NewGVN.cpp
index 19ac9526b5f88b..6770ffd1244098 100644
--- a/llvm/lib/Transforms/Scalar/NewGVN.cpp
+++ b/llvm/lib/Transforms/Scalar/NewGVN.cpp
@@ -297,7 +297,8 @@ class CongruenceClass {
using MemoryMemberSet = SmallPtrSet<const MemoryMemberType *, 2>;
explicit CongruenceClass(unsigned ID) : ID(ID) {}
- CongruenceClass(unsigned ID, Value *Leader, const Expression *E)
+ CongruenceClass(unsigned ID, std::pair<Value *, unsigned int> Leader,
+ const Expression *E)
: ID(ID), RepLeader(Leader), DefiningExpr(E) {}
unsigned getID() const { return ID; }
@@ -311,15 +312,21 @@ class CongruenceClass {
}
// Leader functions
- Value *getLeader() const { return RepLeader; }
- void setLeader(Value *Leader) { RepLeader = Leader; }
+ Value *getLeader() const { return RepLeader.first; }
+ void setLeader(std::pair<Value *, unsigned int> Leader) {
+ RepLeader = Leader;
+ }
const std::pair<Value *, unsigned int> &getNextLeader() const {
return NextLeader;
}
void resetNextLeader() { NextLeader = {nullptr, ~0}; }
- void addPossibleNextLeader(std::pair<Value *, unsigned int> LeaderPair) {
- if (LeaderPair.second < NextLeader.second)
+ void addPossibleLeader(std::pair<Value *, unsigned int> LeaderPair) {
+ if (LeaderPair.second < RepLeader.second) {
+ NextLeader = RepLeader;
+ RepLeader = LeaderPair;
+ } else if (LeaderPair.second < NextLeader.second) {
NextLeader = LeaderPair;
+ }
}
Value *getStoredValue() const { return RepStoredValue; }
@@ -374,9 +381,10 @@ class CongruenceClass {
if (this == Other)
return true;
- if (std::tie(StoreCount, RepLeader, RepStoredValue, RepMemoryAccess) !=
- std::tie(Other->StoreCount, Other->RepLeader, Other->RepStoredValue,
- Other->RepMemoryAccess))
+ if (std::tie(StoreCount, RepLeader.first, RepStoredValue,
+ RepMemoryAccess) !=
+ std::tie(Other->StoreCount, Other->RepLeader.first,
+ Other->RepStoredValue, Other->RepMemoryAccess))
return false;
if (DefiningExpr != Other->DefiningExpr)
if (!DefiningExpr || !Other->DefiningExpr ||
@@ -393,7 +401,7 @@ class CongruenceClass {
unsigned ID;
// Representative leader.
- Value *RepLeader = nullptr;
+ std::pair<Value *, unsigned int> RepLeader = {nullptr, ~0U};
// The most dominating leader after our current leader, because the member set
// is not sorted and is expensive to keep sorted all the time.
@@ -731,7 +739,13 @@ class NewGVN {
// Congruence class handling.
CongruenceClass *createCongruenceClass(Value *Leader, const Expression *E) {
- auto *result = new CongruenceClass(NextCongruenceNum++, Leader, E);
+ unsigned LeaderDFS = 0;
+ if (!Leader)
+ LeaderDFS = ~0;
+ else if (auto *I = dyn_cast<Instruction>(Leader))
+ LeaderDFS = InstrToDFSNum(I);
+ auto *result =
+ new CongruenceClass(NextCongruenceNum++, {Leader, LeaderDFS}, E);
CongruenceClasses.emplace_back(result);
return result;
}
@@ -2245,7 +2259,8 @@ void NewGVN::moveValueToNewCongruenceClass(Instruction *I, const Expression *E,
NewClass->insert(I);
if (NewClass->getLeader() != I)
- NewClass->addPossibleNextLeader({I, InstrToDFSNum(I)});
+ NewClass->addPossibleLeader({I, InstrToDFSNum(I)});
+
// Handle our special casing of stores.
if (auto *SI = dyn_cast<StoreInst>(I)) {
OldClass->decStoreCount();
@@ -2269,7 +2284,7 @@ void NewGVN::moveValueToNewCongruenceClass(Instruction *I, const Expression *E,
<< " because store joined class\n");
// If we changed the leader, we have to mark it changed because we don't
// know what it will do to symbolic evaluation.
- NewClass->setLeader(SI);
+ NewClass->setLeader({SI, InstrToDFSNum(SI)});
}
// We rely on the code below handling the MemoryAccess change.
}
@@ -2315,7 +2330,8 @@ void NewGVN::moveValueToNewCongruenceClass(Instruction *I, const Expression *E,
if (OldClass->getStoredValue())
OldClass->setStoredValue(nullptr);
}
- OldClass->setLeader(getNextValueLeader(OldClass));
+ OldClass->setLeader({getNextValueLeader(OldClass),
+ InstrToDFSNum(getNextValueLeader(OldClass))});
OldClass->resetNextLeader();
markValueLeaderChangeTouched(OldClass);
}
@@ -2354,15 +2370,15 @@ void NewGVN::performCongruenceFinding(Instruction *I, const Expression *E) {
// Constants and variables should always be made the leader.
if (const auto *CE = dyn_cast<ConstantExpression>(E)) {
- NewClass->setLeader(CE->getConstantValue());
+ NewClass->setLeader({CE->getConstantValue(), 0});
} else if (const auto *SE = dyn_cast<StoreExpression>(E)) {
StoreInst *SI = SE->getStoreInst();
- NewClass->setLeader(SI);
+ NewClass->setLeader({SI, InstrToDFSNum(SI)});
NewClass->setStoredValue(SE->getStoredValue());
// The RepMemoryAccess field will be filled in properly by the
// moveValueToNewCongruenceClass call.
} else {
- NewClass->setLeader(I);
+ NewClass->setLeader({I, InstrToDFSNum(I)});
}
assert(!isa<VariableExpression>(E) &&
"VariableExpression should have been handled already");
@@ -3253,7 +3269,6 @@ void NewGVN::verifyMemoryCongruency() const {
return ReachableEdges.count(
{FirstMP->getIncomingBlock(U), FirstMP->getBlock()}) &&
isa<MemoryDef>(U);
-
};
// All arguments should in the same class, ignoring unreachable arguments
auto FilteredPhiArgs =
diff --git a/llvm/test/Transforms/NewGVN/todo-pr36335-phi-undef.ll b/llvm/test/Transforms/NewGVN/pr36335-phi-undef.ll
similarity index 86%
rename from llvm/test/Transforms/NewGVN/todo-pr36335-phi-undef.ll
rename to llvm/test/Transforms/NewGVN/pr36335-phi-undef.ll
index f3f3d2bccb68e2..839e7175b813c7 100644
--- a/llvm/test/Transforms/NewGVN/todo-pr36335-phi-undef.ll
+++ b/llvm/test/Transforms/NewGVN/pr36335-phi-undef.ll
@@ -1,8 +1,6 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt -passes=newgvn -S %s | FileCheck %s
-; TODO: NewGVN currently miscomiles the function below. PR36335.
-
declare void @foo(i32)
define void @main(i1 %c1, i1 %c2, i32 %x) {
@@ -11,7 +9,8 @@ define void @main(i1 %c1, i1 %c2, i32 %x) {
; CHECK-NEXT: br i1 [[C1:%.*]], label [[L:%.*]], label [[END:%.*]]
; CHECK: L:
; CHECK-NEXT: [[XOR:%.*]] = xor i32 [[X:%.*]], -1
-; CHECK-NEXT: call void @foo(i32 [[XOR]])
+; CHECK-NEXT: [[NEG:%.*]] = xor i32 [[XOR]], -1
+; CHECK-NEXT: call void @foo(i32 [[NEG]])
; CHECK-NEXT: br label [[L]]
; CHECK: end:
; CHECK-NEXT: ret void
diff --git a/llvm/test/Transforms/NewGVN/pr69211.ll b/llvm/test/Transforms/NewGVN/pr69211.ll
new file mode 100644
index 00000000000000..dd6a0ec438657f
--- /dev/null
+++ b/llvm/test/Transforms/NewGVN/pr69211.ll
@@ -0,0 +1,58 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -passes=newgvn -S | FileCheck %s
+
+define i32 @main() {
+; CHECK-LABEL: @main(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br label [[FOR_BODY9:%.*]]
+; CHECK: for.body9:
+; CHECK-NEXT: [[INDVARS_IV:%.*]] = phi i64 [ 0, [[ENTRY:%.*]] ], [ [[TMP0:%.*]], [[FOR_BODY9]] ]
+; CHECK-NEXT: [[TMP0]] = add i64 1, [[INDVARS_IV]]
+; CHECK-NEXT: [[TMP1:%.*]] = add i64 [[TMP0]], 1
+; CHECK-NEXT: br label [[FOR_BODY9]]
+;
+entry:
+ br label %for.body9
+
+for.body9: ; preds = %for.body9, %entry
+ %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body9 ]
+ %0 = add i64 1, %indvars.iv
+ %1 = add i64 %0, 1
+ %2 = trunc i64 %1 to i32
+ %indvars.iv.next = add i64 %indvars.iv, 1
+ br label %for.body9
+}
+
+declare void @dummy(i64)
+
+define i32 @foo() {
+; CHECK-LABEL: @foo(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br label [[FOR_BODY:%.*]]
+; CHECK: for.body:
+; CHECK-NEXT: [[INDVARS_IV:%.*]] = phi i64 [ 0, [[ENTRY:%.*]] ], [ [[TMP0:%.*]], [[FOR_BODY]] ]
+; CHECK-NEXT: [[TMP0]] = add i64 [[INDVARS_IV]], 1
+; CHECK-NEXT: [[TMP1:%.*]] = add i64 [[TMP0]], 1
+; CHECK-NEXT: call void @dummy(i64 [[TMP1]])
+; CHECK-NEXT: [[TOBOOL:%.*]] = icmp slt i64 [[TMP0]], 1000
+; CHECK-NEXT: br i1 [[TOBOOL]], label [[FOR_BODY]], label [[END:%.*]]
+; CHECK: end:
+; CHECK-NEXT: ret i32 0
+;
+entry:
+ br label %for.body
+
+for.body: ; preds = %for.body, %entry
+ %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body ]
+ %0 = add i64 %indvars.iv, 1
+ %1 = add i64 %0, 1
+ call void @dummy(i64 %1)
+
+ %indvars.iv.next = add i64 %indvars.iv, 1
+ %tobool = icmp slt i64 %indvars.iv.next, 1000
+ br i1 %tobool, label %for.body, label %end
+
+end:
+ ret i32 0
+}
+
diff --git a/llvm/test/Transforms/NewGVN/simp-to-self.ll b/llvm/test/Transforms/NewGVN/simp-to-self.ll
index fb8a01962959bc..97d70b52103b36 100644
--- a/llvm/test/Transforms/NewGVN/simp-to-self.ll
+++ b/llvm/test/Transforms/NewGVN/simp-to-self.ll
@@ -1,13 +1,24 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 3
; RUN: opt -S < %s -passes=newgvn | FileCheck %s
-; CHECK-LABEL: for.cond:
-; CHECK-NEXT: %lv = load i32, ptr @a
-; CHECK-NEXT: %bf.clear = and i32 %lv, -131072
-; CHECK-NEXT: %bf.set = or i32 1, %bf.clear
-; CHECK-NEXT: br i1 %bc, label %for.cond, label %exit
@a = external global i64
define void @fn1(i1 %bc) {
+; CHECK-LABEL: define void @fn1(
+; CHECK-SAME: i1 [[BC:%.*]]) {
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br label [[FOR_COND:%.*]]
+; CHECK: for.cond:
+; CHECK-NEXT: [[LV:%.*]] = load i32, ptr @a, align 4
+; CHECK-NEXT: [[BF_CLEAR:%.*]] = and i32 [[LV]], -131072
+; CHECK-NEXT: [[BF_SET:%.*]] = or i32 1, [[BF_CLEAR]]
+; CHECK-NEXT: [[BF_CLEAR_1:%.*]] = and i32 [[BF_SET]], -131072
+; CHECK-NEXT: [[BF_SET_1:%.*]] = or i32 1, [[BF_CLEAR_1]]
+; CHECK-NEXT: br i1 [[BC]], label [[FOR_COND]], label [[EXIT:%.*]]
+; CHECK: exit:
+; CHECK-NEXT: store i32 [[BF_SET_1]], ptr @a, align 4
+; CHECK-NEXT: ret void
+;
entry:
br label %for.cond
``````````
</details>
https://github.com/llvm/llvm-project/pull/82110
More information about the llvm-commits
mailing list