Skip to content

[MSSAUpdater] Replace recursion with worklist and cap it. #150543

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[MSSAUpdater] Replace recursion with worklist and cap it. #150543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5c8002c
[MSSAUpdater] Replace recursion with worklist and cap it.
alinas Jul 24, 2025
d25c8c0
Limit condition.
alinas Jul 24, 2025
54f46c2
Remove ValueHandles.
alinas Jul 28, 2025
2d62af4
clang-format
alinas Jul 28, 2025
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
1 change: 0 additions & 1 deletion llvm/include/llvm/Analysis/MemorySSAUpdater.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,6 @@ class MemorySSAUpdater {
MemoryAccess *
getPreviousDefRecursive(BasicBlock *,
DenseMap<BasicBlock *, TrackingVH<MemoryAccess>> &);
MemoryAccess *recursePhi(MemoryAccess *Phi);
MemoryAccess *tryRemoveTrivialPhi(MemoryPhi *Phi);
template <class RangeType>
MemoryAccess *tryRemoveTrivialPhi(MemoryPhi *Phi, RangeType &Operands);
Expand Down
79 changes: 64 additions & 15 deletions llvm/lib/Analysis/MemorySSAUpdater.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,17 @@
#include "llvm/Analysis/MemorySSA.h"
#include "llvm/IR/BasicBlock.h"
#include "llvm/IR/Dominators.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Debug.h"
#include <algorithm>

#define DEBUG_TYPE "memoryssa"
using namespace llvm;

static cl::opt<uint32_t> TrivialPhiProcessingLimit(
"mssaupdater-processing-limit", cl::Hidden, cl::init(20),
cl::desc("The worklist limit for trivial phi removal (default = 20)"));

// This is the marker algorithm from "Simple and Efficient Construction of
// Static Single Assignment Form"
// The simple, non-marker algorithm places phi nodes at any join
Expand Down Expand Up @@ -181,18 +186,6 @@ MemoryAccess *MemorySSAUpdater::getPreviousDefFromEnd(

return getPreviousDefRecursive(BB, CachedPreviousDef);
}
// Recurse over a set of phi uses to eliminate the trivial ones
MemoryAccess *MemorySSAUpdater::recursePhi(MemoryAccess *Phi) {
if (!Phi)
return nullptr;
TrackingVH<MemoryAccess> Res(Phi);
SmallVector<TrackingVH<Value>, 8> Uses;
std::copy(Phi->user_begin(), Phi->user_end(), std::back_inserter(Uses));
for (auto &U : Uses)
if (MemoryPhi *UsePhi = dyn_cast<MemoryPhi>(&*U))
tryRemoveTrivialPhi(UsePhi);
return Res;
}

// Eliminate trivial phis
// Phis are trivial if they are defined either by themselves, or all the same
Expand Down Expand Up @@ -230,9 +223,65 @@ MemoryAccess *MemorySSAUpdater::tryRemoveTrivialPhi(MemoryPhi *Phi,
removeMemoryAccess(Phi);
}

// We should only end up recursing in case we replaced something, in which
// case, we may have made other Phis trivial.
return recursePhi(Same);
// Continue traversal in a DFS worklist approach, in case we might find
// other trivial Phis.
if (!Same)
return nullptr;

// Worklist approach to recursively removing trivial Phis.
SmallVector<Value *, 5> Worklist;
SmallSetVector<Value *, 5> Visited;

for (auto U : Same->users()) {
if (dyn_cast<MemoryPhi>(&*U))
Worklist.push_back(&*U);
}

while (!Worklist.empty() && Worklist.size() < TrivialPhiProcessingLimit) {
MemoryPhi *RecPhi = dyn_cast<MemoryPhi>(&*(Worklist[Worklist.size() - 1]));
Worklist.pop_back();

if (!RecPhi || Visited.contains(RecPhi))
continue;
auto RecOperands = RecPhi->operands();

// Repeat above algorithm.
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of repeating it, can we handle the base case by starting with a one element worklist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the issue here is that this API is used in two scenarios: 1) when looking at a normal MemoryPhi (called from tryRemoveTrivialPhi with the phi's operands), in which case this is just repeating the algorithm, and 2) when looking at an empty MemoryPhi with potential operands (during an update) and the goal is deciding whether to actually complete that Phi. This second case is also the only scenario where the return value is used.
Because the two cases have different types for how the list of operands is represented, this is templated to use a range, and this is also why this was originally a recursive function, so the range type can change (it only really changes at most once though).

I don't know how to resolve this without some duplication while avoiding copying the operands. Here's an alternative, but I'm open to suggestions.

// common case, public API
tryRemoveTrivialPhi(Phi) {
// calls tryRemoveTrivialPhiHelper and discards its return value, void
tryRemoveTrivialPhiHelper
}
// uncommon case, public API, remove template
tryRemoveIncompelteTrivialPhi(Phi, SmallVector<>& PhiOps) {
// duplicates the algorithm for the incoming range, before calling the helper
// returns the value from the helper
tryRemoveTrivialPhiHelper
}
// private API, returns a Phi
tryRemoveTrivialPhiHelper(Phi) {
// worklist algorithm using the Phi's operands, no repetition here
}

WDYT?

if (NonOptPhis.count(RecPhi))
continue;

bool nextone = false;
MemoryAccess *RecSame = nullptr;
for (auto &Op : RecOperands) {
if (Op == RecPhi || Op == RecSame)
continue;
if (RecSame) {
nextone = true;
break;
}
RecSame = cast<MemoryAccess>(&*Op);
}

// Move on to the next Phi in the worklist.
if (nextone || RecSame == nullptr)
continue;

if (RecPhi) {
if (RecPhi == Same) {
// Update the returned value
Same = RecSame;
}
Visited.insert(RecPhi);
RecPhi->replaceAllUsesWith(RecSame);
removeMemoryAccess(RecPhi);
}

for (auto U : RecSame->users()) {
if (dyn_cast<MemoryPhi>(&*U))
Worklist.push_back(&*U);
}
}

return Same;
}

void MemorySSAUpdater::insertUse(MemoryUse *MU, bool RenameUses) {
Expand Down