[MSSAU] Clarify that the defining access does not matter

[nikic]

Summary

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.

[asbirlea]

LGTM.
Another alternative is creating a createMemoryAccess API which does not take the defining access, but likely not worth it.
I'm fairly certain we cannot keep the defining access when inserting Defs in the general case, in particular when needing to insert new Phis.

llvm/lib/Transforms/Scalar/GVN.cpp

2017–2018

Update or remove comment.

[nikic]

Another alternative is creating a createMemoryAccess API which does not take the defining access, but likely not worth it.

We may be able to remove the Definition argument from these APIs entirely. I believe the only caller that supplies Definition now is https://github.com/llvm/llvm-project/blob/9d2f8ecac88c73242c99e7b353ac985fc57f1ed3/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp#L323-L325, so we would only need an alternative for that one.

llvm/include/llvm/Analysis/MemorySSAUpdater.h

void wireOldPredecessorsToNewImmediatePredecessor(
       BasicBlock *Old, BasicBlock *New, ArrayRef<BasicBlock *> Preds,
       bool IdenticalEdgesWereMerged = true);
   // The below are utility functions. Other than creation of accesses to pass
   // to insertDef, and removeAccess to remove accesses, you should generally
   // not attempt to update memoryssa yourself. It is very non-trivial to get
   // 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);
 
   /// Remove a MemoryAccess from MemorySSA, including updating all
   /// definitions and uses.
   /// This should be called when a memory instruction that has a MemoryAccess
   /// associated with it is erased from the program.  For example, if a store or

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp

bool tryFoldIntoCalloc(MemoryDef *Def, const Value *DefUO) {
     auto *Calloc = emitCalloc(ConstantInt::get(SizeTTy, 1),
                               Malloc->getArgOperand(0), IRB, TLI);
     if (!Calloc)
       return false;
     MemorySSAUpdater Updater(&MSSA);
     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);
     Updater.removeMemoryAccess(Malloc);
     Malloc->replaceAllUsesWith(Calloc);
     Malloc->eraseFromParent();
     return true;
   }

llvm/lib/Transforms/Scalar/GVN.cpp

for (const auto &AvailableLoad : AvailableLoads) {
     Value *LoadPtr = AvailableLoad.second;
 
     auto *NewLoad =
         new LoadInst(Load->getType(), LoadPtr, Load->getName() + ".pre",
                      Load->isVolatile(), Load->getAlign(), Load->getOrdering(),
                      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
         MSSAU->insertUse(cast<MemoryUse>(NewAccess), /*RenameUses=*/true);
     }
 
     // Transfer the old load's AA tags to the new load.
     AAMDNodes Tags = Load->getAAMetadata();
if (Cond->isZero()) {
           for (const auto &Acc : *AL) {
             if (auto *Current = dyn_cast<MemoryUseOrDef>(&Acc))
               if (!Current->getMemoryInst()->comesBefore(NewS)) {
                 FirstNonDom = Current;
                 break;
               }
           }
         }
 
-        // 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);
       }
     }
     if (isAssumeWithEmptyBundle(*IntrinsicI)) {
       markInstructionForDeletion(IntrinsicI);
       return true;

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

Instruction *MemCpyOptPass::tryMergingIntoMemset(Instruction *StartInst,
   // Join these together into ranges, so we can decide whether contiguous blocks
   // are stored.
   MemsetRanges Ranges(DL);
 
   BasicBlock::iterator BI(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.
     if (auto *CB = dyn_cast<CallBase>(BI)) {
       if (CB->onlyAccessesInaccessibleMemory())
         continue;
     }
 
for (const MemsetRange &Range : Ranges) {
 
     LLVM_DEBUG(dbgs() << "Replace stores:\n"; for (Instruction *SI
                                                    : Range.TheStores) dbgs()
                                               << *SI << '\n';
                dbgs() << "With: " << *AMemSet << '\n');
     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.
     for (Instruction *SI : Range.TheStores)
       eraseInstruction(SI);
 
     ++NumMemSetInfer;
   }
if (P) {
             LI->getPointerOperand(), LI->getAlign(), Size);
       M->copyMetadata(*SI, LLVMContext::MD_DIAssignID);
 
       LLVM_DEBUG(dbgs() << "Promoting " << *LI << " to " << *SI << " => "
                         << *M << "\n");
 
       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);
       eraseInstruction(LI);
       ++NumMemCpyInstr;
 
       // Make sure we do not invalidate the iterator.
       BBI = M->getIterator();
if (T->isAggregateType()) {
       M->copyMetadata(*SI, LLVMContext::MD_DIAssignID);
 
       LLVM_DEBUG(dbgs() << "Promoting " << *SI << " to " << *M << "\n");
 
       // The newly inserted memset is immediately overwritten by the original
       // 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);
       NumMemSetInfer++;
 
       // Make sure we do not invalidate the iterator.
       BBI = M->getIterator();
       return true;
bool MemCpyOptPass::processMemCpyMemCpyDependence(MemCpyInst *M,
   } else
     NewM = Builder.CreateMemCpy(M->getRawDest(), M->getDestAlign(),
                                 MDep->getRawSource(), MDep->getSourceAlign(),
                                 M->getLength(), M->isVolatile());
   NewM->copyMetadata(*M, LLVMContext::MD_DIAssignID);
 
   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.
   eraseInstruction(M);
   ++NumMemCpyInstr;
   return true;
 }
 
bool MemCpyOptPass::processMemSetMemCpyDependence(MemCpyInst *MemCpy,
 
   assert(isa<MemoryDef>(MSSAU->getMemorySSA()->getMemoryAccess(MemCpy)) &&
          "MemCpy must be a MemoryDef");
   // The new memset is inserted before the memcpy, and it is known that the
   // memcpy's defining access is the memset about to be removed.
   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);
   return true;
 }
 
 /// Determine whether the instruction has undefined content for the given Size,
 /// either because it was freshly alloca'd or started its lifetime.
bool MemCpyOptPass::performMemCpyToMemSetOptzn(MemCpyInst *MemCpy,
   }
 
   IRBuilder<> Builder(MemCpy);
   Instruction *NewM =
       Builder.CreateMemSet(MemCpy->getRawDest(), MemSet->getOperand(1),
                            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;
 }
 
 // Attempts to optimize the pattern whereby memory is copied from an alloca to
 // another alloca, where the two allocas don't have conflicting mod/ref. If
 // successful, the two allocas can be merged into one and the transfer can be
if (!LifetimeMarkers.empty()) {
     IRBuilder<> Builder(C);
 
     ConstantInt *AllocaSize = ConstantInt::get(Type::getInt64Ty(C), Size);
     // Create a new lifetime start marker before the first user of src or alloca
     // users.
     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
     // users.
     // FIXME: If the last user is the terminator for the bb, we can insert
     // lifetime.end marker to the immidiate post-dominator, but currently do
     // nothing.
     if (!LastUser->isTerminator()) {
       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);
     }
 
     // Remove all other lifetime markers.
     for (Instruction *I : LifetimeMarkers)
       eraseInstruction(I);
   }
 
if (GV->isConstant() && GV->hasDefinitiveInitializer())
       if (Value *ByteVal = isBytewiseValue(GV->getInitializer(),
                                            M->getModule()->getDataLayout())) {
         IRBuilder<> Builder(M);
         Instruction *NewM = Builder.CreateMemSet(
             M->getRawDest(), ByteVal, M->getLength(), M->getDestAlign(), false);
         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);
         ++NumCpyToSet;
         return true;
       }
 
   BatchAAResults BAA(*AA);