[llvm] 829992c - [NewGVN] Prevent cyclic dependencies by ensuring Leader has min RPO number (#82110)
via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 14 00:18:38 PDT 2024
Author: ManuelJBrito
Date: 2024-08-14T08:18:33+01:00
New Revision: 829992cf21e9220bbf7985073745ee8f09b0b7f1
URL: https://github.com/llvm/llvm-project/commit/829992cf21e9220bbf7985073745ee8f09b0b7f1
DIFF: https://github.com/llvm/llvm-project/commit/829992cf21e9220bbf7985073745ee8f09b0b7f1.diff
LOG: [NewGVN] Prevent cyclic dependencies by ensuring Leader has min RPO number (#82110)
Cyclic dependencies in NewGVN can result in miscompilation and
termination issues.
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.
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.
Added:
llvm/test/Transforms/NewGVN/pr36335-phi-undef.ll
Modified:
llvm/lib/Transforms/Scalar/NewGVN.cpp
llvm/test/Transforms/NewGVN/simp-to-self.ll
Removed:
llvm/test/Transforms/NewGVN/todo-pr36335-phi-undef.ll
################################################################################
diff --git a/llvm/lib/Transforms/Scalar/NewGVN.cpp b/llvm/lib/Transforms/Scalar/NewGVN.cpp
index fc0b31c433962d..13d9e8f186b47c 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,23 @@ 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)
+ bool addPossibleLeader(std::pair<Value *, unsigned int> LeaderPair) {
+ if (LeaderPair.second < RepLeader.second) {
+ NextLeader = RepLeader;
+ RepLeader = LeaderPair;
+ return true;
+ } else if (LeaderPair.second < NextLeader.second) {
NextLeader = LeaderPair;
+ }
+ return false;
}
Value *getStoredValue() const { return RepStoredValue; }
@@ -392,11 +401,13 @@ class CongruenceClass {
private:
unsigned ID;
- // Representative leader.
- Value *RepLeader = nullptr;
+ // Representative leader and its corresponding RPO number.
+ // The leader must have the lowest RPO number.
+ 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.
+ // The most dominating leader after our current leader (given by the RPO
+ // number), because the member set is not sorted and is expensive to keep
+ // sorted all the time.
std::pair<Value *, unsigned int> NextLeader = {nullptr, ~0U};
// If this is represented by a store, the value of the store.
@@ -735,7 +746,19 @@ class NewGVN {
// Congruence class handling.
CongruenceClass *createCongruenceClass(Value *Leader, const Expression *E) {
- auto *result = new CongruenceClass(NextCongruenceNum++, Leader, E);
+ // Set RPO to 0 for values that are always available (constants and function
+ // args). These should always be made leader.
+ unsigned LeaderDFS = 0;
+
+ // If Leader is not specified, either we have a memory class or the leader
+ // will be set later. Otherwise, if Leader is an Instruction, set LeaderDFS
+ // to its RPO number.
+ 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;
}
@@ -2248,8 +2271,13 @@ void NewGVN::moveValueToNewCongruenceClass(Instruction *I, const Expression *E,
OldClass->erase(I);
NewClass->insert(I);
- if (NewClass->getLeader() != I)
- NewClass->addPossibleNextLeader({I, InstrToDFSNum(I)});
+ // Ensure that the leader has the lowest RPO. If the leader changed notify all
+ // members of the class.
+ if (NewClass->getLeader() != I &&
+ NewClass->addPossibleLeader({I, InstrToDFSNum(I)})) {
+ markValueLeaderChangeTouched(NewClass);
+ }
+
// Handle our special casing of stores.
if (auto *SI = dyn_cast<StoreInst>(I)) {
OldClass->decStoreCount();
@@ -2273,7 +2301,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.
}
@@ -2319,7 +2347,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);
}
@@ -2358,15 +2387,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 +3282,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/simp-to-self.ll b/llvm/test/Transforms/NewGVN/simp-to-self.ll
index f9a0ec257259a1..2bedc77b5fbfdc 100644
--- a/llvm/test/Transforms/NewGVN/simp-to-self.ll
+++ b/llvm/test/Transforms/NewGVN/simp-to-self.ll
@@ -12,9 +12,11 @@ define void @fn1(i1 %bc) {
; 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]], ptr @a, align 4
+; CHECK-NEXT: store i32 [[BF_SET_1]], ptr @a, align 4
; CHECK-NEXT: ret void
;
entry:
More information about the llvm-commits
mailing list