Skip to content

[GVN] Refactor the LeaderTable structure into a properly encapsulated data structure #88347

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 26, 2024
Merged

[GVN] Refactor the LeaderTable structure into a properly encapsulated data structure #88347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 60 additions & 51 deletions llvm/include/llvm/Transforms/Scalar/GVN.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,13 +232,67 @@ class GVNPass : public PassInfoMixin<GVNPass> {

/// A mapping from value numbers to lists of Value*'s that
/// have that value number. Use findLeader to query it.
struct LeaderTableEntry {
Value *Val;
const BasicBlock *BB;
LeaderTableEntry *Next;
class LeaderMap {
public:
struct LeaderTableEntry {
Value *Val;
const BasicBlock *BB;
};

private:
struct LeaderListNode {
LeaderTableEntry Entry;
LeaderListNode *Next;
};
DenseMap<uint32_t, LeaderListNode> NumToLeaders;
BumpPtrAllocator TableAllocator;

public:
class leader_iterator {
const LeaderListNode *Current;

public:
using iterator_category = std::forward_iterator_tag;
using value_type = const LeaderTableEntry;
using difference_type = std::ptrdiff_t;
using pointer = value_type *;
using reference = value_type &;

leader_iterator(const LeaderListNode *C) : Current(C) {}
leader_iterator &operator++() {
assert(Current && "Dereferenced end of leader list!");
Current = Current->Next;
return *this;
}
bool operator==(const leader_iterator &Other) const {
return Current == Other.Current;
}
bool operator!=(const leader_iterator &Other) const {
return Current != Other.Current;
}
reference operator*() const { return Current->Entry; }
};

iterator_range<leader_iterator> getLeaders(uint32_t N) {
auto I = NumToLeaders.find(N);
if (I == NumToLeaders.end()) {
return iterator_range(leader_iterator(nullptr),
leader_iterator(nullptr));
}

return iterator_range(leader_iterator(&I->second),
leader_iterator(nullptr));
}

void insert(uint32_t N, Value *V, const BasicBlock *BB);
void erase(uint32_t N, Instruction *I, const BasicBlock *BB);
void verifyRemoved(const Value *Inst) const;
void clear() {
NumToLeaders.clear();
TableAllocator.Reset();
}
};
DenseMap<uint32_t, LeaderTableEntry> LeaderTable;
BumpPtrAllocator TableAllocator;
LeaderMap LeaderTable;

// Block-local map of equivalent values to their leader, does not
// propagate to any successors. Entries added mid-block are applied
Expand All @@ -264,51 +318,6 @@ class GVNPass : public PassInfoMixin<GVNPass> {
MemoryDependenceResults *RunMD, LoopInfo &LI,
OptimizationRemarkEmitter *ORE, MemorySSA *MSSA = nullptr);

/// Push a new Value to the LeaderTable onto the list for its value number.
void addToLeaderTable(uint32_t N, Value *V, const BasicBlock *BB) {
LeaderTableEntry &Curr = LeaderTable[N];
if (!Curr.Val) {
Curr.Val = V;
Curr.BB = BB;
return;
}

LeaderTableEntry *Node = TableAllocator.Allocate<LeaderTableEntry>();
Node->Val = V;
Node->BB = BB;
Node->Next = Curr.Next;
Curr.Next = Node;
}

/// Scan the list of values corresponding to a given
/// value number, and remove the given instruction if encountered.
void removeFromLeaderTable(uint32_t N, Instruction *I, BasicBlock *BB) {
LeaderTableEntry *Prev = nullptr;
LeaderTableEntry *Curr = &LeaderTable[N];

while (Curr && (Curr->Val != I || Curr->BB != BB)) {
Prev = Curr;
Curr = Curr->Next;
}

if (!Curr)
return;

if (Prev) {
Prev->Next = Curr->Next;
} else {
if (!Curr->Next) {
Curr->Val = nullptr;
Curr->BB = nullptr;
} else {
LeaderTableEntry *Next = Curr->Next;
Curr->Val = Next->Val;
Curr->BB = Next->BB;
Curr->Next = Next->Next;
}
}
}

// List of critical edges to be split between iterations.
SmallVector<std::pair<Instruction *, unsigned>, 4> toSplit;

Expand Down
135 changes: 89 additions & 46 deletions llvm/lib/Transforms/Scalar/GVN.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,6 +725,69 @@ void GVNPass::ValueTable::verifyRemoved(const Value *V) const {
"Inst still occurs in value numbering map!");
}

//===----------------------------------------------------------------------===//
// LeaderMap External Functions
//===----------------------------------------------------------------------===//

/// Push a new Value to the LeaderTable onto the list for its value number.
void GVNPass::LeaderMap::insert(uint32_t N, Value *V, const BasicBlock *BB) {
LeaderListNode &Curr = NumToLeaders[N];
if (!Curr.Entry.Val) {
Curr.Entry.Val = V;
Curr.Entry.BB = BB;
return;
}

LeaderListNode *Node = TableAllocator.Allocate<LeaderListNode>();
Node->Entry.Val = V;
Node->Entry.BB = BB;
Node->Next = Curr.Next;
Curr.Next = Node;
}

/// Scan the list of values corresponding to a given
/// value number, and remove the given instruction if encountered.
void GVNPass::LeaderMap::erase(uint32_t N, Instruction *I,
const BasicBlock *BB) {
LeaderListNode *Prev = nullptr;
LeaderListNode *Curr = &NumToLeaders[N];

while (Curr && (Curr->Entry.Val != I || Curr->Entry.BB != BB)) {
Prev = Curr;
Curr = Curr->Next;
}

if (!Curr)
return;

if (Prev) {
Prev->Next = Curr->Next;
} else {
if (!Curr->Next) {
Curr->Entry.Val = nullptr;
Curr->Entry.BB = nullptr;
} else {
LeaderListNode *Next = Curr->Next;
Curr->Entry.Val = Next->Entry.Val;
Curr->Entry.BB = Next->Entry.BB;
Curr->Next = Next->Next;
}
}
}

void GVNPass::LeaderMap::verifyRemoved(const Value *V) const {
// Walk through the value number scope to make sure the instruction isn't
// ferreted away in it.
for (const auto &I : NumToLeaders) {
(void)I;
assert(I.second.Entry.Val != V && "Inst still in value numbering scope!");
assert(
std::none_of(leader_iterator(&I.second), leader_iterator(nullptr),
[=](const LeaderTableEntry &E) { return E.Val == V; }) &&
"Inst still in value numbering scope!");
}
}

//===----------------------------------------------------------------------===//
// GVN Pass
//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -1467,7 +1530,7 @@ void GVNPass::eliminatePartiallyRedundantLoad(
OldLoad->replaceAllUsesWith(NewLoad);
replaceValuesPerBlockEntry(ValuesPerBlock, OldLoad, NewLoad);
if (uint32_t ValNo = VN.lookup(OldLoad, false))
removeFromLeaderTable(ValNo, OldLoad, OldLoad->getParent());
LeaderTable.erase(ValNo, OldLoad, OldLoad->getParent());
VN.erase(OldLoad);
removeInstruction(OldLoad);
}
Expand Down Expand Up @@ -2204,10 +2267,9 @@ GVNPass::ValueTable::assignExpNewValueNum(Expression &Exp) {
/// defined in \p BB.
bool GVNPass::ValueTable::areAllValsInBB(uint32_t Num, const BasicBlock *BB,
GVNPass &Gvn) {
LeaderTableEntry *Vals = &Gvn.LeaderTable[Num];
while (Vals && Vals->BB == BB)
Vals = Vals->Next;
return !Vals;
return all_of(
Gvn.LeaderTable.getLeaders(Num),
[=](const LeaderMap::LeaderTableEntry &L) { return L.BB == BB; });
}

/// Wrap phiTranslateImpl to provide caching functionality.
Expand All @@ -2229,12 +2291,11 @@ bool GVNPass::ValueTable::areCallValsEqual(uint32_t Num, uint32_t NewNum,
const BasicBlock *PhiBlock,
GVNPass &Gvn) {
CallInst *Call = nullptr;
LeaderTableEntry *Vals = &Gvn.LeaderTable[Num];
while (Vals) {
Call = dyn_cast<CallInst>(Vals->Val);
auto Leaders = Gvn.LeaderTable.getLeaders(Num);
for (const auto &Entry : Leaders) {
Call = dyn_cast<CallInst>(Entry.Val);
if (Call && Call->getParent() == PhiBlock)
break;
Vals = Vals->Next;
}

if (AA->doesNotAccessMemory(Call))
Expand Down Expand Up @@ -2327,23 +2388,17 @@ void GVNPass::ValueTable::eraseTranslateCacheEntry(
// question. This is fast because dominator tree queries consist of only
// a few comparisons of DFS numbers.
Value *GVNPass::findLeader(const BasicBlock *BB, uint32_t num) {
LeaderTableEntry Vals = LeaderTable[num];
if (!Vals.Val) return nullptr;
auto Leaders = LeaderTable.getLeaders(num);
if (Leaders.empty())
return nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we don't need this explicit check?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that it's probably unneeded, but it preserves the original semantics of if (!Vals.Val) return nullptr;. I'd prefer to avoid avoid changing the semantics so as not to introduce unintended functional changes in this patch.


Value *Val = nullptr;
if (DT->dominates(Vals.BB, BB)) {
Val = Vals.Val;
if (isa<Constant>(Val)) return Val;
}

LeaderTableEntry* Next = Vals.Next;
while (Next) {
if (DT->dominates(Next->BB, BB)) {
if (isa<Constant>(Next->Val)) return Next->Val;
if (!Val) Val = Next->Val;
for (const auto &Entry : Leaders) {
if (DT->dominates(Entry.BB, BB)) {
Val = Entry.Val;
if (isa<Constant>(Val))
return Val;
}

Next = Next->Next;
}

return Val;
Expand Down Expand Up @@ -2452,7 +2507,7 @@ bool GVNPass::propagateEquality(Value *LHS, Value *RHS,
// have the simple case where the edge dominates the end.
if (RootDominatesEnd && !isa<Instruction>(RHS) &&
canReplacePointersIfEqual(LHS, RHS, DL))
addToLeaderTable(LVN, RHS, Root.getEnd());
LeaderTable.insert(LVN, RHS, Root.getEnd());

// Replace all occurrences of 'LHS' with 'RHS' everywhere in the scope. As
// LHS always has at least one use that is not dominated by Root, this will
Expand Down Expand Up @@ -2546,7 +2601,7 @@ bool GVNPass::propagateEquality(Value *LHS, Value *RHS,
// The leader table only tracks basic blocks, not edges. Only add to if we
// have the simple case where the edge dominates the end.
if (RootDominatesEnd)
addToLeaderTable(Num, NotVal, Root.getEnd());
LeaderTable.insert(Num, NotVal, Root.getEnd());

continue;
}
Expand Down Expand Up @@ -2596,7 +2651,7 @@ bool GVNPass::processInstruction(Instruction *I) {
return true;

unsigned Num = VN.lookupOrAdd(Load);
addToLeaderTable(Num, Load, Load->getParent());
LeaderTable.insert(Num, Load, Load->getParent());
return false;
}

Expand Down Expand Up @@ -2664,15 +2719,15 @@ bool GVNPass::processInstruction(Instruction *I) {
// Allocations are always uniquely numbered, so we can save time and memory
// by fast failing them.
if (isa<AllocaInst>(I) || I->isTerminator() || isa<PHINode>(I)) {
addToLeaderTable(Num, I, I->getParent());
LeaderTable.insert(Num, I, I->getParent());
return false;
}

// If the number we were assigned was a brand new VN, then we don't
// need to do a lookup to see if the number already exists
// somewhere in the domtree: it can't!
if (Num >= NextNum) {
addToLeaderTable(Num, I, I->getParent());
LeaderTable.insert(Num, I, I->getParent());
return false;
}

Expand All @@ -2681,7 +2736,7 @@ bool GVNPass::processInstruction(Instruction *I) {
Value *Repl = findLeader(I->getParent(), Num);
if (!Repl) {
// Failure, just remember this instance for future use.
addToLeaderTable(Num, I, I->getParent());
LeaderTable.insert(Num, I, I->getParent());
return false;
}

Expand Down Expand Up @@ -2876,7 +2931,7 @@ bool GVNPass::performScalarPREInsertion(Instruction *Instr, BasicBlock *Pred,
VN.add(Instr, Num);

// Update the availability map to include the new instruction.
addToLeaderTable(Num, Instr, Pred);
LeaderTable.insert(Num, Instr, Pred);
return true;
}

Expand Down Expand Up @@ -3027,13 +3082,13 @@ bool GVNPass::performScalarPRE(Instruction *CurInst) {
// After creating a new PHI for ValNo, the phi translate result for ValNo will
// be changed, so erase the related stale entries in phi translate cache.
VN.eraseTranslateCacheEntry(ValNo, *CurrentBlock);
addToLeaderTable(ValNo, Phi, CurrentBlock);
LeaderTable.insert(ValNo, Phi, CurrentBlock);
Phi->setDebugLoc(CurInst->getDebugLoc());
CurInst->replaceAllUsesWith(Phi);
if (MD && Phi->getType()->isPtrOrPtrVectorTy())
MD->invalidateCachedPointerInfo(Phi);
VN.erase(CurInst);
removeFromLeaderTable(ValNo, CurInst, CurrentBlock);
LeaderTable.erase(ValNo, CurInst, CurrentBlock);

LLVM_DEBUG(dbgs() << "GVN PRE removed: " << *CurInst << '\n');
removeInstruction(CurInst);
Expand Down Expand Up @@ -3127,7 +3182,6 @@ void GVNPass::cleanupGlobalSets() {
VN.clear();
LeaderTable.clear();
BlockRPONumber.clear();
TableAllocator.Reset();
ICF->clear();
InvalidBlockRPONumbers = true;
}
Expand All @@ -3147,18 +3201,7 @@ void GVNPass::removeInstruction(Instruction *I) {
/// internal data structures.
void GVNPass::verifyRemoved(const Instruction *Inst) const {
VN.verifyRemoved(Inst);

// Walk through the value number scope to make sure the instruction isn't
// ferreted away in it.
for (const auto &I : LeaderTable) {
const LeaderTableEntry *Node = &I.second;
assert(Node->Val != Inst && "Inst still in value numbering scope!");

while (Node->Next) {
Node = Node->Next;
assert(Node->Val != Inst && "Inst still in value numbering scope!");
}
}
LeaderTable.verifyRemoved(Inst);
}

/// BB is declared dead, which implied other blocks become dead as well. This
Expand Down Expand Up @@ -3285,7 +3328,7 @@ void GVNPass::assignValNumForDeadCode() {
for (BasicBlock *BB : DeadBlocks) {
for (Instruction &Inst : *BB) {
unsigned ValNum = VN.lookupOrAdd(&Inst);
addToLeaderTable(ValNum, &Inst, BB);
LeaderTable.insert(ValNum, &Inst, BB);
}
}
}
Expand Down