[llvm] [NewGVN] Prevent cyclic dependencies by ensuring Leader has min RPO number (PR #82110)
via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 11 17:12:12 PDT 2024
https://github.com/ManuelJBrito updated https://github.com/llvm/llvm-project/pull/82110
>From 16217ceef7be121c025ea8b9d9aed49daba42f60 Mon Sep 17 00:00:00 2001
From: ManuelJBrito <manuel.brito at tecnico.ulisboa.pt>
Date: Fri, 12 Jul 2024 01:11:42 +0100
Subject: [PATCH] RPO Leader
---
llvm/lib/Transforms/Scalar/NewGVN.cpp | 64 +++++++++++++------
...6335-phi-undef.ll => pr36335-phi-undef.ll} | 5 +-
llvm/test/Transforms/NewGVN/simp-to-self.ll | 4 +-
3 files changed, 51 insertions(+), 22 deletions(-)
rename llvm/test/Transforms/NewGVN/{todo-pr36335-phi-undef.ll => pr36335-phi-undef.ll} (86%)
diff --git a/llvm/lib/Transforms/Scalar/NewGVN.cpp b/llvm/lib/Transforms/Scalar/NewGVN.cpp
index 4cba196ed688a..87e0dc0e1a00d 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.
@@ -731,7 +742,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;
}
@@ -2244,8 +2267,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();
@@ -2269,7 +2297,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 +2343,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 +2383,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 f3f3d2bccb68e..839e7175b813c 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 f9a0ec257259a..2bedc77b5fbfd 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