Skip to content

Commit

Permalink
[MSSAU] Clarify that the defining access does not matter (NFC)
Browse files Browse the repository at this point in the history 
New memory accesses are usually inserted by using one of the
createMemoryAccessXYZ() methods followed by insertUse() or
insertDef(). createMemoryAccessXYZ() accepts a defining access,
however this defining access will always be overwritten by
insertUse() / insertDef().

Update the documentation to clarify this, and stop passing
Definition to createMemoryAccessXYZ() if it's followed by
insertUse/insertDef.

Alternatively, we could also make insertUse / insertDef keep the
defining access if it is specified, and only recompute it if it's
missing.

Differential Revision: https://reviews.llvm.org/D157979
  • Loading branch information
@nikic
nikic committed Aug 16, 2023
1 parent 3cf54c5 commit 9d2f8ec
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 60 deletions.
35 changes: 16 additions & 19 deletions llvm/include/llvm/Analysis/MemorySSAUpdater.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,36 +174,33 @@ class MemorySSAUpdater {
// the edge cases right, and the above calls already operate in near-optimal
// time bounds.

/// Create a MemoryAccess in MemorySSA at a specified point in a block,
/// with a specified clobbering definition.
/// Create a MemoryAccess in MemorySSA at a specified point in a block.
///
/// When used by itself, this method will only insert the new MemoryAccess
/// into the access list, but not make any other changes, such as inserting
/// MemoryPHI nodes, or updating users to point to the new MemoryAccess. You
/// must specify a correct Definition in this case.
///
/// Usually, this API is instead combined with insertUse() or insertDef(),
/// which will perform all the necessary MSSA updates. If these APIs are used,
/// then nullptr can be used as Definition, as the correct defining access
/// will be automatically determined.
///
/// Returns the new MemoryAccess.
/// This should be called when a memory instruction is created that is being
/// used to replace an existing memory instruction. It will *not* create PHI
/// nodes, or verify the clobbering definition. The insertion place is used
/// solely to determine where in the memoryssa access lists the instruction
/// will be placed. The caller is expected to keep ordering the same as
/// instructions.
/// It will return the new MemoryAccess.
/// Note: If a MemoryAccess already exists for I, this function will make it
/// inaccessible and it *must* have removeMemoryAccess called on it.
MemoryAccess *createMemoryAccessInBB(Instruction *I, MemoryAccess *Definition,
const BasicBlock *BB,
MemorySSA::InsertionPlace Point);

/// Create a MemoryAccess in MemorySSA before or after an existing
/// MemoryAccess.
///
/// Returns the new MemoryAccess.
/// This should be called when a memory instruction is created that is being
/// used to replace an existing memory instruction. It will *not* create PHI
/// nodes, or verify the clobbering definition.
/// Create a MemoryAccess in MemorySSA before an existing MemoryAccess.
///
/// Note: If a MemoryAccess already exists for I, this function will make it
/// inaccessible and it *must* have removeMemoryAccess called on it.
/// See createMemoryAccessInBB() for usage details.
MemoryUseOrDef *createMemoryAccessBefore(Instruction *I,
MemoryAccess *Definition,
MemoryUseOrDef *InsertPt);
/// Create a MemoryAccess in MemorySSA after an existing MemoryAccess.
///
/// See createMemoryAccessInBB() for usage details.
MemoryUseOrDef *createMemoryAccessAfter(Instruction *I,
MemoryAccess *Definition,
MemoryAccess *InsertPt);
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1897,7 +1897,7 @@ struct DSEState {
auto *LastDef =
cast<MemoryDef>(Updater.getMemorySSA()->getMemoryAccess(Malloc));
auto *NewAccess =
Updater.createMemoryAccessAfter(cast<Instruction>(Calloc), LastDef,
Updater.createMemoryAccessAfter(cast<Instruction>(Calloc), nullptr,
LastDef);
auto *NewAccessMD = cast<MemoryDef>(NewAccess);
Updater.insertDef(NewAccessMD, /*RenameUses=*/true);
Expand Down
16 changes: 3 additions & 13 deletions llvm/lib/Transforms/Scalar/GVN.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1416,16 +1416,8 @@ void GVNPass::eliminatePartiallyRedundantLoad(
Load->getSyncScopeID(), UnavailableBlock->getTerminator());
NewLoad->setDebugLoc(Load->getDebugLoc());
if (MSSAU) {
auto *MSSA = MSSAU->getMemorySSA();
// Get the defining access of the original load or use the load if it is a
// MemoryDef (e.g. because it is volatile). The inserted loads are
// guaranteed to load from the same definition.
auto *LoadAcc = MSSA->getMemoryAccess(Load);
auto *DefiningAcc =
isa<MemoryDef>(LoadAcc) ? LoadAcc : LoadAcc->getDefiningAccess();
auto *NewAccess = MSSAU->createMemoryAccessInBB(
NewLoad, DefiningAcc, NewLoad->getParent(),
MemorySSA::BeforeTerminator);
NewLoad, nullptr, NewLoad->getParent(), MemorySSA::BeforeTerminator);
if (auto *NewDef = dyn_cast<MemoryDef>(NewAccess))
MSSAU->insertDef(NewDef, /*RenameUses=*/true);
else
Expand Down Expand Up @@ -2023,14 +2015,12 @@ bool GVNPass::processAssumeIntrinsic(AssumeInst *IntrinsicI) {
}
}

// This added store is to null, so it will never executed and we can
// just use the LiveOnEntry def as defining access.
auto *NewDef =
FirstNonDom ? MSSAU->createMemoryAccessBefore(
NewS, MSSAU->getMemorySSA()->getLiveOnEntryDef(),
NewS, nullptr,
const_cast<MemoryUseOrDef *>(FirstNonDom))
: MSSAU->createMemoryAccessInBB(
NewS, MSSAU->getMemorySSA()->getLiveOnEntryDef(),
NewS, nullptr,
NewS->getParent(), MemorySSA::BeforeTerminator);

MSSAU->insertDef(cast<MemoryDef>(NewDef), /*RenameUses=*/false);
Expand Down
39 changes: 12 additions & 27 deletions llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,21 +357,13 @@ Instruction *MemCpyOptPass::tryMergingIntoMemset(Instruction *StartInst,

// Keeps track of the last memory use or def before the insertion point for
// the new memset. The new MemoryDef for the inserted memsets will be inserted
// after MemInsertPoint. It points to either LastMemDef or to the last user
// before the insertion point of the memset, if there are any such users.
// after MemInsertPoint.
MemoryUseOrDef *MemInsertPoint = nullptr;
// Keeps track of the last MemoryDef between StartInst and the insertion point
// for the new memset. This will become the defining access of the inserted
// memsets.
MemoryDef *LastMemDef = nullptr;
for (++BI; !BI->isTerminator(); ++BI) {
auto *CurrentAcc = cast_or_null<MemoryUseOrDef>(
MSSAU->getMemorySSA()->getMemoryAccess(&*BI));
if (CurrentAcc) {
if (CurrentAcc)
MemInsertPoint = CurrentAcc;
if (auto *CurrentDef = dyn_cast<MemoryDef>(CurrentAcc))
LastMemDef = CurrentDef;
}

// Calls that only access inaccessible memory do not block merging
// accessible stores.
Expand Down Expand Up @@ -475,16 +467,13 @@ Instruction *MemCpyOptPass::tryMergingIntoMemset(Instruction *StartInst,
if (!Range.TheStores.empty())
AMemSet->setDebugLoc(Range.TheStores[0]->getDebugLoc());

assert(LastMemDef && MemInsertPoint &&
"Both LastMemDef and MemInsertPoint need to be set");
auto *NewDef =
cast<MemoryDef>(MemInsertPoint->getMemoryInst() == &*BI
? MSSAU->createMemoryAccessBefore(
AMemSet, LastMemDef, MemInsertPoint)
AMemSet, nullptr, MemInsertPoint)
: MSSAU->createMemoryAccessAfter(
AMemSet, LastMemDef, MemInsertPoint));
AMemSet, nullptr, MemInsertPoint));
MSSAU->insertDef(NewDef, /*RenameUses=*/true);
LastMemDef = NewDef;
MemInsertPoint = NewDef;

// Zap all the stores.
Expand Down Expand Up @@ -693,7 +682,7 @@ bool MemCpyOptPass::processStoreOfLoad(StoreInst *SI, LoadInst *LI,

auto *LastDef =
cast<MemoryDef>(MSSAU->getMemorySSA()->getMemoryAccess(SI));
auto *NewAccess = MSSAU->createMemoryAccessAfter(M, LastDef, LastDef);
auto *NewAccess = MSSAU->createMemoryAccessAfter(M, nullptr, LastDef);
MSSAU->insertDef(cast<MemoryDef>(NewAccess), /*RenameUses=*/true);

eraseInstruction(SI);
Expand Down Expand Up @@ -814,7 +803,7 @@ bool MemCpyOptPass::processStore(StoreInst *SI, BasicBlock::iterator &BBI) {
// store, so we do not need to rename uses.
auto *StoreDef = cast<MemoryDef>(MSSA->getMemoryAccess(SI));
auto *NewAccess = MSSAU->createMemoryAccessBefore(
M, StoreDef->getDefiningAccess(), StoreDef);
M, nullptr, StoreDef);
MSSAU->insertDef(cast<MemoryDef>(NewAccess), /*RenameUses=*/false);

eraseInstruction(SI);
Expand Down Expand Up @@ -1203,7 +1192,7 @@ bool MemCpyOptPass::processMemCpyMemCpyDependence(MemCpyInst *M,

assert(isa<MemoryDef>(MSSAU->getMemorySSA()->getMemoryAccess(M)));
auto *LastDef = cast<MemoryDef>(MSSAU->getMemorySSA()->getMemoryAccess(M));
auto *NewAccess = MSSAU->createMemoryAccessAfter(NewM, LastDef, LastDef);
auto *NewAccess = MSSAU->createMemoryAccessAfter(NewM, nullptr, LastDef);
MSSAU->insertDef(cast<MemoryDef>(NewAccess), /*RenameUses=*/true);

// Remove the instruction we're replacing.
Expand Down Expand Up @@ -1315,7 +1304,7 @@ bool MemCpyOptPass::processMemSetMemCpyDependence(MemCpyInst *MemCpy,
auto *LastDef =
cast<MemoryDef>(MSSAU->getMemorySSA()->getMemoryAccess(MemCpy));
auto *NewAccess = MSSAU->createMemoryAccessBefore(
NewMemSet, LastDef->getDefiningAccess(), LastDef);
NewMemSet, nullptr, LastDef);
MSSAU->insertDef(cast<MemoryDef>(NewAccess), /*RenameUses=*/true);

eraseInstruction(MemSet);
Expand Down Expand Up @@ -1420,7 +1409,7 @@ bool MemCpyOptPass::performMemCpyToMemSetOptzn(MemCpyInst *MemCpy,
CopySize, MemCpy->getDestAlign());
auto *LastDef =
cast<MemoryDef>(MSSAU->getMemorySSA()->getMemoryAccess(MemCpy));
auto *NewAccess = MSSAU->createMemoryAccessAfter(NewM, LastDef, LastDef);
auto *NewAccess = MSSAU->createMemoryAccessAfter(NewM, nullptr, LastDef);
MSSAU->insertDef(cast<MemoryDef>(NewAccess), /*RenameUses=*/true);

return true;
Expand Down Expand Up @@ -1610,8 +1599,7 @@ bool MemCpyOptPass::performStackMoveOptzn(Instruction *Load, Instruction *Store,
Builder.SetInsertPoint(FirstUser->getParent(), FirstUser->getIterator());
auto *Start = Builder.CreateLifetimeStart(SrcAlloca, AllocaSize);
auto *FirstMA = MSSA->getMemoryAccess(FirstUser);
auto *StartMA = MSSAU->createMemoryAccessBefore(
Start, FirstMA->getDefiningAccess(), FirstMA);
auto *StartMA = MSSAU->createMemoryAccessBefore(Start, nullptr, FirstMA);
MSSAU->insertDef(cast<MemoryDef>(StartMA), /*RenameUses=*/true);

// Create a new lifetime end marker after the last user of src or alloca
Expand All @@ -1623,10 +1611,7 @@ bool MemCpyOptPass::performStackMoveOptzn(Instruction *Load, Instruction *Store,
Builder.SetInsertPoint(LastUser->getParent(), ++LastUser->getIterator());
auto *End = Builder.CreateLifetimeEnd(SrcAlloca, AllocaSize);
auto *LastMA = MSSA->getMemoryAccess(LastUser);
// FIXME: the second argument should be LastMA if LastMA is MemoryDef, but
// that's updated by insertDef.
auto *EndMA = MSSAU->createMemoryAccessAfter(
End, LastMA->getDefiningAccess(), LastMA);
auto *EndMA = MSSAU->createMemoryAccessAfter(End, nullptr, LastMA);
MSSAU->insertDef(cast<MemoryDef>(EndMA), /*RenameUses=*/true);
}

Expand Down Expand Up @@ -1674,7 +1659,7 @@ bool MemCpyOptPass::processMemCpy(MemCpyInst *M, BasicBlock::iterator &BBI) {
auto *LastDef =
cast<MemoryDef>(MSSAU->getMemorySSA()->getMemoryAccess(M));
auto *NewAccess =
MSSAU->createMemoryAccessAfter(NewM, LastDef, LastDef);
MSSAU->createMemoryAccessAfter(NewM, nullptr, LastDef);
MSSAU->insertDef(cast<MemoryDef>(NewAccess), /*RenameUses=*/true);

eraseInstruction(M);
Expand Down

0 comments on commit 9d2f8ec

Please sign in to comment.