[llvm] [SSAUpdaterBulk] Fix incorrect live-in values for a block. (PR #131762)
via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 18 01:52:42 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-llvm-transforms
Author: Valery Pykhtin (vpykhtin)
<details>
<summary>Changes</summary>
The previous implementation incorrectly calculated incoming values from loop backedges, as demonstrated by the tests. The issue was that it did not distinguish between live-in and live-out values for blocks. This patch addresses the problem and fixes https://github.com/llvm/llvm-project/pull/131761.
To avoid bloating storage in `R.Defines`, processing data has been moved to a temporary map `BBInfos`. This change helps manage heap allocation more efficiently and likely improves caching.
This patch will be followed by a patch that adds phi-values simplification making SSAUpdaterBulk a drop-in replacement for SSAUpdater. This would make https://github.com/llvm/llvm-project/pull/130611 pass lit tests without their modification.
---
Full diff: https://github.com/llvm/llvm-project/pull/131762.diff
3 Files Affected:
- (modified) llvm/include/llvm/Transforms/Utils/SSAUpdaterBulk.h (+1-3)
- (modified) llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp (+52-27)
- (modified) llvm/unittests/Transforms/Utils/SSAUpdaterBulkTest.cpp (+117)
``````````diff
diff --git a/llvm/include/llvm/Transforms/Utils/SSAUpdaterBulk.h b/llvm/include/llvm/Transforms/Utils/SSAUpdaterBulk.h
index ad24cb454d5e7..b2cf29608f58b 100644
--- a/llvm/include/llvm/Transforms/Utils/SSAUpdaterBulk.h
+++ b/llvm/include/llvm/Transforms/Utils/SSAUpdaterBulk.h
@@ -38,7 +38,7 @@ class DominatorTree;
/// is used).
class SSAUpdaterBulk {
struct RewriteInfo {
- DenseMap<BasicBlock *, Value *> Defines;
+ SmallVector<std::pair<BasicBlock *, Value *>, 4> Defines;
SmallVector<Use *, 4> Uses;
StringRef Name;
Type *Ty;
@@ -49,8 +49,6 @@ class SSAUpdaterBulk {
PredIteratorCache PredCache;
- Value *computeValueAt(BasicBlock *BB, RewriteInfo &R, DominatorTree *DT);
-
public:
explicit SSAUpdaterBulk() = default;
SSAUpdaterBulk(const SSAUpdaterBulk &) = delete;
diff --git a/llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp b/llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp
index cad7ff64c01fb..0e689319b8fac 100644
--- a/llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp
+++ b/llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp
@@ -53,7 +53,7 @@ void SSAUpdaterBulk::AddAvailableValue(unsigned Var, BasicBlock *BB, Value *V) {
LLVM_DEBUG(dbgs() << "SSAUpdater: Var=" << Var
<< ": added new available value " << *V << " in "
<< BB->getName() << "\n");
- Rewrites[Var].Defines[BB] = V;
+ Rewrites[Var].Defines.emplace_back(BB, V);
}
/// Record a use of the symbolic value. This use will be updated with a
@@ -65,21 +65,6 @@ void SSAUpdaterBulk::AddUse(unsigned Var, Use *U) {
Rewrites[Var].Uses.push_back(U);
}
-// Compute value at the given block BB. We either should already know it, or we
-// should be able to recursively reach it going up dominator tree.
-Value *SSAUpdaterBulk::computeValueAt(BasicBlock *BB, RewriteInfo &R,
- DominatorTree *DT) {
- if (!R.Defines.count(BB)) {
- if (DT->isReachableFromEntry(BB) && PredCache.get(BB).size()) {
- BasicBlock *IDom = DT->getNode(BB)->getIDom()->getBlock();
- Value *V = computeValueAt(IDom, R, DT);
- R.Defines[BB] = V;
- } else
- R.Defines[BB] = UndefValue::get(R.Ty);
- }
- return R.Defines[BB];
-}
-
/// Given sets of UsingBlocks and DefBlocks, compute the set of LiveInBlocks.
/// This is basically a subgraph limited by DefBlocks and UsingBlocks.
static void
@@ -117,11 +102,19 @@ ComputeLiveInBlocks(const SmallPtrSetImpl<BasicBlock *> &UsingBlocks,
}
}
+struct BBValueInfo {
+ Value *LiveInValue = nullptr;
+ Value *LiveOutValue = nullptr;
+};
+
/// Perform all the necessary updates, including new PHI-nodes insertion and the
/// requested uses update.
void SSAUpdaterBulk::RewriteAllUses(DominatorTree *DT,
SmallVectorImpl<PHINode *> *InsertedPHIs) {
+ DenseMap<BasicBlock *, BBValueInfo> BBInfos;
for (auto &R : Rewrites) {
+ BBInfos.clear();
+
// Compute locations for new phi-nodes.
// For that we need to initialize DefBlocks from definitions in R.Defines,
// UsingBlocks from uses in R.Uses, then compute LiveInBlocks, and then use
@@ -132,8 +125,8 @@ void SSAUpdaterBulk::RewriteAllUses(DominatorTree *DT,
<< " use(s)\n");
SmallPtrSet<BasicBlock *, 2> DefBlocks;
- for (auto &Def : R.Defines)
- DefBlocks.insert(Def.first);
+ for (auto [BB, V] : R.Defines)
+ DefBlocks.insert(BB);
IDF.setDefiningBlocks(DefBlocks);
SmallPtrSet<BasicBlock *, 2> UsingBlocks;
@@ -143,26 +136,55 @@ void SSAUpdaterBulk::RewriteAllUses(DominatorTree *DT,
SmallVector<BasicBlock *, 32> IDFBlocks;
SmallPtrSet<BasicBlock *, 32> LiveInBlocks;
ComputeLiveInBlocks(UsingBlocks, DefBlocks, LiveInBlocks, PredCache);
- IDF.resetLiveInBlocks();
IDF.setLiveInBlocks(LiveInBlocks);
IDF.calculate(IDFBlocks);
+ // Reserve map large enough to reduce growth.
+ BBInfos.init(LiveInBlocks.size() + DefBlocks.size());
+
+ for (auto [BB, V] : R.Defines)
+ BBInfos[BB].LiveOutValue = V;
+
// We've computed IDF, now insert new phi-nodes there.
- SmallVector<PHINode *, 4> InsertedPHIsForVar;
for (auto *FrontierBB : IDFBlocks) {
IRBuilder<> B(FrontierBB, FrontierBB->begin());
PHINode *PN = B.CreatePHI(R.Ty, 0, R.Name);
- R.Defines[FrontierBB] = PN;
- InsertedPHIsForVar.push_back(PN);
+ BBInfos[FrontierBB].LiveInValue = PN;
if (InsertedPHIs)
InsertedPHIs->push_back(PN);
}
+ // IsLiveOut indicates whether we are computing live-out values (true) or
+ // live-in values (false).
+ std::function<Value *(BasicBlock *, bool)> computeValue =
+ [&](BasicBlock *BB, bool IsLiveOut) -> Value * {
+ auto &BBInfo = BBInfos[BB];
+
+ if (IsLiveOut && BBInfo.LiveOutValue)
+ return BBInfo.LiveOutValue;
+
+ if (BBInfo.LiveInValue)
+ return BBInfo.LiveInValue;
+
+ Value *V = DT->isReachableFromEntry(BB) && PredCache.get(BB).size()
+ ? computeValue(DT->getNode(BB)->getIDom()->getBlock(),
+ /* IsLiveOut = */ true)
+ : UndefValue::get(R.Ty);
+
+ // The call to computeValue for the dominator block can insert another
+ // entry into the map, potentially causing the map to grow and
+ // invalidating the BBInfo reference. Therefore, we need to perform
+ // another map lookup. Simply reserving map size may not be sufficient
+ // as the map could grow further.
+ BBInfos[BB].LiveInValue = V;
+ return V;
+ };
+
// Fill in arguments of the inserted PHIs.
- for (auto *PN : InsertedPHIsForVar) {
- BasicBlock *PBB = PN->getParent();
- for (BasicBlock *Pred : PredCache.get(PBB))
- PN->addIncoming(computeValueAt(Pred, R, DT), Pred);
+ for (auto *BB : IDFBlocks) {
+ auto *PHI = cast<PHINode>(&BB->front());
+ for (BasicBlock *Pred : PredCache.get(BB))
+ PHI->addIncoming(computeValue(Pred, /* IsLiveOut = */ true), Pred);
}
// Rewrite actual uses with the inserted definitions.
@@ -170,7 +192,10 @@ void SSAUpdaterBulk::RewriteAllUses(DominatorTree *DT,
for (Use *U : R.Uses) {
if (!ProcessedUses.insert(U).second)
continue;
- Value *V = computeValueAt(getUserBB(U), R, DT);
+
+ auto *User = cast<Instruction>(U->getUser());
+ BasicBlock *BB = getUserBB(U);
+ Value *V = computeValue(BB, /* IsLiveOut = */ BB != User->getParent());
Value *OldVal = U->get();
assert(OldVal && "Invalid use!");
// Notify that users of the existing value that it is being replaced.
diff --git a/llvm/unittests/Transforms/Utils/SSAUpdaterBulkTest.cpp b/llvm/unittests/Transforms/Utils/SSAUpdaterBulkTest.cpp
index b75a492c58bc4..6913e4d978aa1 100644
--- a/llvm/unittests/Transforms/Utils/SSAUpdaterBulkTest.cpp
+++ b/llvm/unittests/Transforms/Utils/SSAUpdaterBulkTest.cpp
@@ -14,7 +14,9 @@
#include "llvm/IR/Instructions.h"
#include "llvm/IR/LLVMContext.h"
#include "llvm/IR/Module.h"
+#include "llvm/Support/SourceMgr.h"
#include "gtest/gtest.h"
+#include "gtest/gtest-spi.h"
using namespace llvm;
@@ -192,3 +194,118 @@ TEST(SSAUpdaterBulk, Irreducible) {
EXPECT_EQ(UpdatePhi->getIncomingValueForBlock(LoopStartBB), AddOp2);
EXPECT_EQ(UpdatePhi->getIncomingValueForBlock(IfBB), UndefValue::get(I32Ty));
}
+
+TEST(SSAUpdaterBulk, SingleBBLoop) {
+ const char *IR = R"(
+ define void @main() {
+ entry:
+ br label %loop
+ loop:
+ %i = add i32 0, 1
+ %cmp = icmp slt i32 %i, 42
+ br i1 %cmp, label %loop, label %exit
+ exit:
+ ret void
+ }
+ )";
+
+ llvm::LLVMContext Context;
+ llvm::SMDiagnostic Err;
+ std::unique_ptr<llvm::Module> M = llvm::parseAssemblyString(IR, Err, Context);
+ ASSERT_NE(M, nullptr) << "Failed to parse IR: " << Err.getMessage();
+
+ Function *F = M->getFunction("main");
+ auto *Entry = &F->getEntryBlock();
+ auto *Loop = Entry->getSingleSuccessor();
+ auto *I = &Loop->front();
+
+ // Rewrite first operand of "%i = add i32 0, 1" to use incoming values entry:0
+ // or loop:%i (that is the value of %i from the previous iteration).
+ SSAUpdaterBulk Updater;
+ Type *I32Ty = Type::getInt32Ty(Context);
+ unsigned PrevI = Updater.AddVariable("i.prev", I32Ty);
+ Updater.AddAvailableValue(PrevI, Entry, ConstantInt::get(I32Ty, 0));
+ Updater.AddAvailableValue(PrevI, Loop, I);
+ Updater.AddUse(PrevI, &I->getOperandUse(0));
+
+ SmallVector<PHINode *, 1> Inserted;
+ DominatorTree DT(*F);
+ Updater.RewriteAllUses(&DT, &Inserted);
+
+#if 0 // Enable for debugging.
+ Loop->dump();
+ // Output:
+ // loop: ; preds = %loop, %entry
+ // %i.prev = phi i32 [ %i, %loop ], [ 0, %entry ]
+ // %i = add i32 %i.prev, 1
+ // %cmp = icmp slt i32 %i, 42
+ // br i1 %cmp, label %loop, label %exit
+#endif
+
+ ASSERT_EQ(Inserted.size(), 1u);
+ PHINode *Phi = Inserted[0];
+ EXPECT_EQ(Phi, dyn_cast<PHINode>(I->getOperand(0)));
+ EXPECT_EQ(Phi->getIncomingValueForBlock(Entry), ConstantInt::get(I32Ty, 0));
+ EXPECT_EQ(Phi->getIncomingValueForBlock(Loop), I);
+}
+
+TEST(SSAUpdaterBulk, TwoBBLoop) {
+ const char *IR = R"(
+ define void @main() {
+ entry:
+ br label %loop_header
+ loop_header:
+ br label %loop
+ loop:
+ %i = add i32 0, 1
+ %cmp = icmp slt i32 %i, 42
+ br i1 %cmp, label %loop_header, label %exit
+ exit:
+ ret void
+ }
+ )";
+
+ llvm::LLVMContext Context;
+ llvm::SMDiagnostic Err;
+ std::unique_ptr<llvm::Module> M = llvm::parseAssemblyString(IR, Err, Context);
+ ASSERT_NE(M, nullptr) << "Failed to parse IR: " << Err.getMessage();
+
+ Function *F = M->getFunction("main");
+ auto *Entry = &F->getEntryBlock();
+ auto *LoopHdr = Entry->getSingleSuccessor();
+ auto *Loop = LoopHdr->getSingleSuccessor();
+ auto *I = &Loop->front();
+
+ // Rewrite first operand of "%i = add i32 0, 1" to use incoming values entry:0
+ // or loop:%i (that is the value of %i from the previous iteration).
+ SSAUpdaterBulk Updater;
+ Type *I32Ty = Type::getInt32Ty(Context);
+ unsigned PrevI = Updater.AddVariable("i.prev", I32Ty);
+ Updater.AddAvailableValue(PrevI, Entry, ConstantInt::get(I32Ty, 0));
+ Updater.AddAvailableValue(PrevI, Loop, I);
+ Updater.AddUse(PrevI, &I->getOperandUse(0));
+
+ SmallVector<PHINode *, 1> Inserted;
+ DominatorTree DT(*F);
+ Updater.RewriteAllUses(&DT, &Inserted);
+
+#if 0 // Enable for debugging.
+ LoopHdr->dump();
+ Loop->dump();
+ // Output:
+ // loop_header: ; preds = %loop, %entry
+ // %i.prev = phi i32 [ %i, %loop ], [ 0, %entry ]
+ // br label %loop
+ // loop: ; preds = %loop_header
+ // %i = add i32 %i.prev, 1
+ // %cmp = icmp slt i32 %i, 42
+ // br i1 %cmp, label %loop_header, label %exit
+#endif
+
+ ASSERT_EQ(Inserted.size(), 1u);
+ PHINode *Phi = Inserted[0];
+ EXPECT_EQ(Phi, dyn_cast<PHINode>(I->getOperand(0)));
+ EXPECT_EQ(Phi->getParent(), LoopHdr);
+ EXPECT_EQ(Phi->getIncomingValueForBlock(Entry), ConstantInt::get(I32Ty, 0));
+ EXPECT_EQ(Phi->getIncomingValueForBlock(Loop), I);
+}
\ No newline at end of file
``````````
</details>
https://github.com/llvm/llvm-project/pull/131762
More information about the llvm-commits
mailing list