[MemCpyOptimizer] Simplify API of processStore and processMem* functions

[foad]

Summary

Previously these functions either returned a "changed" flag or a "repeat
instruction" flag, and could also modify an iterator to control which
instruction would be processed next.

Simplify this by always returning a "changed" flag, and handling all of
the "repeat instruction" functionality by modifying the iterator.

No functional change intended except in this case:
// If the source and destination of the memcpy are the same, then zap it.
... where the previous code failed to process the instruction after the
zapped memcpy.

[serge-sans-paille]

I really like this cleanup step, thanks!

llvm/include/llvm/Transforms/Scalar/MemCpyOptimizer.h

bool runImpl(Function &F, MemoryDependenceResults *MD_,
                std::function<AssumptionCache &()> LookupAssumptionCache_,
                std::function<DominatorTree &()> LookupDomTree_);
 
 private:
   // Helper functions
   bool processStore(StoreInst *SI, BasicBlock::iterator &BBI);
   bool processMemSet(MemSetInst *SI, BasicBlock::iterator &BBI);
   bool processMemCpy(MemCpyInst *M, BasicBlock::iterator &BBI);
-  bool processMemMove(MemMoveInst *M);
+  bool processMemMove(MemMoveInst *M, BasicBlock::iterator &BBI);
   bool performCallSlotOptzn(Instruction *cpy, Value *cpyDst, Value *cpySrc,
                             uint64_t cpyLen, Align cpyAlign, CallInst *C);
-  bool processMemCpyMemCpyDependence(MemCpyInst *M, MemCpyInst *MDep);
-  bool processMemSetMemCpyDependence(MemCpyInst *M, MemSetInst *MDep);
-  bool performMemCpyToMemSetOptzn(MemCpyInst *M, MemSetInst *MDep);
+  Instruction *processMemCpyMemCpyDependence(MemCpyInst *M, MemCpyInst *MDep);
+  Instruction *processMemSetMemCpyDependence(MemCpyInst *M, MemSetInst *MDep);
+  Instruction *performMemCpyToMemSetOptzn(MemCpyInst *M, MemSetInst *MDep);
   bool processByValArgument(CallBase &CB, unsigned ArgNo);
   Instruction *tryMergingIntoMemset(Instruction *I, Value *StartPtr,
                                     Value *ByteVal);
 
   bool iterateOnFunction(Function &F);
 };
 
 } // end namespace llvm
 
 #endif // LLVM_TRANSFORMS_SCALAR_MEMCPYOPTIMIZER_H

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

static bool moveUp(AliasAnalysis &AA, StoreInst *SI, Instruction *P,
   for (auto *I : llvm::reverse(ToLift)) {
     LLVM_DEBUG(dbgs() << "Lifting " << *I << " before " << *P << "\n");
     I->moveBefore(P);
   }
 
   return true;
 }
 
+/// If changes are made, return true and set BBI to the next instruction to
+/// visit.
 bool MemCpyOptPass::processStore(StoreInst *SI, BasicBlock::iterator &BBI) {
   if (!SI->isSimple()) return false;
 
   // Avoid merging nontemporal stores since the resulting
   // memcpy/memset would not be able to preserve the nontemporal hint.
   // In theory we could teach how to propagate the !nontemporal metadata to
   // memset calls. However, that change would force the backend to
   // conservatively expand !nontemporal memset calls back to sequences of
if (LI->isSimple() && LI->hasOneUse() &&
                             << *M << "\n");
 
           MD->removeInstruction(SI);
           SI->eraseFromParent();
           MD->removeInstruction(LI);
           LI->eraseFromParent();
           ++NumMemCpyInstr;
 
-          // Make sure we do not invalidate the iterator.
           BBI = M->getIterator();
           return true;
         }
       }
 
       // Detect cases where we're performing call slot forwarding, but
       // happen to be using a load-store pair to implement it, rather than
       // a memcpy.
bool MemCpyOptPass::processStore(StoreInst *SI, BasicBlock::iterator &BBI) {
 
   // Ensure that the value being stored is something that can be memset'able a
   // byte at a time like "0" or "-1" or any width, as well as things like
   // 0xA0A0A0A0 and 0.0.
   auto *V = SI->getOperand(0);
   if (Value *ByteVal = isBytewiseValue(V, DL)) {
     if (Instruction *I = tryMergingIntoMemset(SI, SI->getPointerOperand(),
                                               ByteVal)) {
-      BBI = I->getIterator(); // Don't invalidate iterator.
+      BBI = I->getIterator();
       return true;
     }
 
     // If we have an aggregate, we try to promote it to memset regardless
     // of opportunity for merging as it can expose optimization opportunities
     // in subsequent passes.
     auto *T = V->getType();
     if (T->isAggregateType()) {
       uint64_t Size = DL.getTypeStoreSize(T);
       IRBuilder<> Builder(SI);
       auto *M = Builder.CreateMemSet(SI->getPointerOperand(), ByteVal, Size,
                                      SI->getAlign());
 
       LLVM_DEBUG(dbgs() << "Promoting " << *SI << " to " << *M << "\n");
 
       MD->removeInstruction(SI);
       SI->eraseFromParent();
       NumMemSetInfer++;
 
-      // Make sure we do not invalidate the iterator.
       BBI = M->getIterator();
       return true;
     }
   }
 
   return false;
 }
 
+/// If changes are made, return true and set BBI to the next instruction to
+/// visit.
 bool MemCpyOptPass::processMemSet(MemSetInst *MSI, BasicBlock::iterator &BBI) {
   // See if there is another memset or store neighboring this memset which
   // allows us to widen out the memset to do a single larger store.
   if (isa<ConstantInt>(MSI->getLength()) && !MSI->isVolatile())
     if (Instruction *I = tryMergingIntoMemset(MSI, MSI->getDest(),
                                               MSI->getValue())) {
-      BBI = I->getIterator(); // Don't invalidate iterator.
+      BBI = I->getIterator();
       return true;
     }
   return false;
 }
 
 /// Takes a memcpy and a call that it depends on,
 /// and checks for the possibility of a call slot optimization by having
 /// the call write its result directly into the destination of the memcpy.
bool MemCpyOptPass::performCallSlotOptzn(Instruction *cpy, Value *cpyDest,
   MD->removeInstruction(cpy);
   ++NumMemCpyInstr;
 
   return true;
 }
 
 /// We've found that the (upward scanning) memory dependence of memcpy 'M' is
 /// the memcpy 'MDep'. Try to simplify M to copy from MDep's input if we can.
-bool MemCpyOptPass::processMemCpyMemCpyDependence(MemCpyInst *M,
+Instruction *MemCpyOptPass::processMemCpyMemCpyDependence(MemCpyInst *M,
                                                           MemCpyInst *MDep) {
   // We can only transforms memcpy's where the dest of one is the source of the
   // other.
   if (M->getSource() != MDep->getDest() || MDep->isVolatile())
-    return false;
+    return nullptr;
 
   // If dep instruction is reading from our current input, then it is a noop
   // transfer and substituting the input won't change this instruction.  Just
   // ignore the input and let someone else zap MDep.  This handles cases like:
   //    memcpy(a <- a)
   //    memcpy(b <- a)
   if (M->getSource() == MDep->getSource())
-    return false;
+    return nullptr;
 
   // Second, the length of the memcpy's must be the same, or the preceding one
   // must be larger than the following one.
   ConstantInt *MDepLen = dyn_cast<ConstantInt>(MDep->getLength());
   ConstantInt *MLen = dyn_cast<ConstantInt>(M->getLength());
   if (!MDepLen || !MLen || MDepLen->getZExtValue() < MLen->getZExtValue())
-    return false;
+    return nullptr;
 
   AliasAnalysis &AA = LookupAliasAnalysis();
 
   // Verify that the copied-from memory doesn't change in between the two
   // transfers.  For example, in:
   //    memcpy(a <- b)
   //    *b = 42;
   //    memcpy(c <- a)
   // It would be invalid to transform the second memcpy into memcpy(c <- b).
   //
   // TODO: If the code between M and MDep is transparent to the destination "c",
   // then we could still perform the xform by moving M up to the first memcpy.
   //
   // NOTE: This is conservative, it will stop on any read from the source loc,
   // not just the defining memcpy.
   MemDepResult SourceDep =
       MD->getPointerDependencyFrom(MemoryLocation::getForSource(MDep), false,
                                    M->getIterator(), M->getParent());
   if (!SourceDep.isClobber() || SourceDep.getInst() != MDep)
-    return false;
+    return nullptr;
 
   // If the dest of the second might alias the source of the first, then the
   // source and dest might overlap.  We still want to eliminate the intermediate
   // value, but we have to generate a memmove instead of memcpy.
   bool UseMemMove = false;
   if (!AA.isNoAlias(MemoryLocation::getForDest(M),
                     MemoryLocation::getForSource(MDep)))
     UseMemMove = true;
 
   // If all checks passed, then we can transform M.
   LLVM_DEBUG(dbgs() << "MemCpyOptPass: Forwarding memcpy->memcpy src:\n"
                     << *MDep << '\n' << *M << '\n');
 
   // TODO: Is this worth it if we're creating a less aligned memcpy? For
   // example we could be moving from movaps -> movq on x86.
   IRBuilder<> Builder(M);
+  Instruction *MC;
   if (UseMemMove)
-    Builder.CreateMemMove(M->getRawDest(), M->getDestAlign(),
+    MC = Builder.CreateMemMove(M->getRawDest(), M->getDestAlign(),
                                MDep->getRawSource(), MDep->getSourceAlign(),
                                M->getLength(), M->isVolatile());
   else
-    Builder.CreateMemCpy(M->getRawDest(), M->getDestAlign(),
+    MC = Builder.CreateMemCpy(M->getRawDest(), M->getDestAlign(),
                               MDep->getRawSource(), MDep->getSourceAlign(),
                               M->getLength(), M->isVolatile());
 
   // Remove the instruction we're replacing.
   MD->removeInstruction(M);
   M->eraseFromParent();
   ++NumMemCpyInstr;
-  return true;
+  return MC;
 }
 
 /// We've found that the (upward scanning) memory dependence of \p MemCpy is
 /// \p MemSet.  Try to simplify \p MemSet to only set the trailing bytes that
 /// weren't copied over by \p MemCpy.
 ///
 /// In other words, transform:
 /// \code
 ///   memset(dst, c, dst_size);
 ///   memcpy(dst, src, src_size);
 /// \endcode
 /// into:
 /// \code
 ///   memcpy(dst, src, src_size);
 ///   memset(dst + src_size, c, dst_size <= src_size ? 0 : dst_size - src_size);
 /// \endcode
-bool MemCpyOptPass::processMemSetMemCpyDependence(MemCpyInst *MemCpy,
+Instruction *MemCpyOptPass::processMemSetMemCpyDependence(MemCpyInst *MemCpy,
                                                           MemSetInst *MemSet) {
   // We can only transform memset/memcpy with the same destination.
   if (MemSet->getDest() != MemCpy->getDest())
-    return false;
+    return nullptr;
 
   // Check that there are no other dependencies on the memset destination.
   MemDepResult DstDepInfo =
       MD->getPointerDependencyFrom(MemoryLocation::getForDest(MemSet), false,
                                    MemCpy->getIterator(), MemCpy->getParent());
   if (DstDepInfo.getInst() != MemSet)
-    return false;
+    return nullptr;
 
   // Use the same i8* dest as the memcpy, killing the memset dest if different.
   Value *Dest = MemCpy->getRawDest();
   Value *DestSize = MemSet->getLength();
   Value *SrcSize = MemCpy->getLength();
 
   // By default, create an unaligned memset.
   unsigned Align = 1;
if (DestSize->getType() != SrcSize->getType()) {
     else
       DestSize = Builder.CreateZExt(DestSize, SrcSize->getType());
   }
 
   Value *Ule = Builder.CreateICmpULE(DestSize, SrcSize);
   Value *SizeDiff = Builder.CreateSub(DestSize, SrcSize);
   Value *MemsetLen = Builder.CreateSelect(
       Ule, ConstantInt::getNullValue(DestSize->getType()), SizeDiff);
-  Builder.CreateMemSet(
+  auto *MS = Builder.CreateMemSet(
       Builder.CreateGEP(Dest->getType()->getPointerElementType(), Dest,
                         SrcSize),
       MemSet->getOperand(1), MemsetLen, MaybeAlign(Align));
 
   MD->removeInstruction(MemSet);
   MemSet->eraseFromParent();
-  return true;
+  return MS;
 }
 
 /// Determine whether the instruction has undefined content for the given Size,
 /// either because it was freshly alloca'd or started its lifetime.
 static bool hasUndefContents(Instruction *I, ConstantInt *Size) {
   if (isa<AllocaInst>(I))
     return true;
 

 /// into:
 /// \code
 ///   memset(dst1, c, dst1_size);
 ///   memset(dst2, c, dst2_size);
 /// \endcode
 /// When dst2_size <= dst1_size.
 ///
 /// The \p MemCpy must have a Constant length.
-bool MemCpyOptPass::performMemCpyToMemSetOptzn(MemCpyInst *MemCpy,
+Instruction *MemCpyOptPass::performMemCpyToMemSetOptzn(MemCpyInst *MemCpy,
                                                        MemSetInst *MemSet) {
   AliasAnalysis &AA = LookupAliasAnalysis();
 
   // Make sure that memcpy(..., memset(...), ...), that is we are memsetting and
   // memcpying from the same address. Otherwise it is hard to reason about.
   if (!AA.isMustAlias(MemSet->getRawDest(), MemCpy->getRawSource()))
-    return false;
+    return nullptr;
 
   // A known memset size is required.
   ConstantInt *MemSetSize = dyn_cast<ConstantInt>(MemSet->getLength());
   if (!MemSetSize)
-    return false;
+    return nullptr;
 
   // Make sure the memcpy doesn't read any more than what the memset wrote.
   // Don't worry about sizes larger than i64.
   ConstantInt *CopySize = cast<ConstantInt>(MemCpy->getLength());
   if (CopySize->getZExtValue() > MemSetSize->getZExtValue()) {
     // If the memcpy is larger than the memset, but the memory was undef prior
     // to the memset, we can just ignore the tail. Technically we're only
     // interested in the bytes from MemSetSize..CopySize here, but as we can't
     // easily represent this location, we use the full 0..CopySize range.
     MemoryLocation MemCpyLoc = MemoryLocation::getForSource(MemCpy);
     MemDepResult DepInfo = MD->getPointerDependencyFrom(
         MemCpyLoc, true, MemSet->getIterator(), MemSet->getParent());
     if (DepInfo.isDef() && hasUndefContents(DepInfo.getInst(), CopySize))
       CopySize = MemSetSize;
     else
-      return false;
+      return nullptr;
   }
 
   IRBuilder<> Builder(MemCpy);
-  Builder.CreateMemSet(MemCpy->getRawDest(), MemSet->getOperand(1), CopySize,
-                       MaybeAlign(MemCpy->getDestAlignment()));
-  return true;
+  return Builder.CreateMemSet(MemCpy->getRawDest(), MemSet->getOperand(1),
+                              CopySize, MaybeAlign(MemCpy->getDestAlignment()));
 }
 
 /// Perform simplification of memcpy's.  If we have memcpy A
 /// which copies X to Y, and memcpy B which copies Y to Z, then we can rewrite
 /// B to be a memcpy from X to Z (or potentially a memmove, depending on
 /// circumstances). This allows later passes to remove the first memcpy
 /// altogether.
+/// If changes are made, return true and set BBI to the next instruction to
+/// visit.
 bool MemCpyOptPass::processMemCpy(MemCpyInst *M, BasicBlock::iterator &BBI) {
   // We can only optimize non-volatile memcpy's.
   if (M->isVolatile()) return false;
 
   // If the source and destination of the memcpy are the same, then zap it.
   if (M->getSource() == M->getDest()) {
-    ++BBI;
     MD->removeInstruction(M);
     M->eraseFromParent();
     return true;
   }
 
   // If copying from a constant, try to turn the memcpy into a memset.
-  if (GlobalVariable *GV = dyn_cast<GlobalVariable>(M->getSource()))
-    if (GV->isConstant() && GV->hasDefinitiveInitializer())
+  if (GlobalVariable *GV = dyn_cast<GlobalVariable>(M->getSource())) {
+    if (GV->isConstant() && GV->hasDefinitiveInitializer()) {
       if (Value *ByteVal = isBytewiseValue(GV->getInitializer(),
                                            M->getModule()->getDataLayout())) {
         IRBuilder<> Builder(M);
+        auto *MS =
             Builder.CreateMemSet(M->getRawDest(), ByteVal, M->getLength(),
                                  MaybeAlign(M->getDestAlignment()), false);
         MD->removeInstruction(M);
         M->eraseFromParent();
         ++NumCpyToSet;
+        BBI = MS->getIterator();
         return true;
       }
+    }
+  }
 
   MemDepResult DepInfo = MD->getDependency(M);
 
   // Try to turn a partially redundant memset + memcpy into
   // memcpy + smaller memset.  We don't need the memcpy size for this.
-  if (DepInfo.isClobber())
-    if (MemSetInst *MDep = dyn_cast<MemSetInst>(DepInfo.getInst()))
-      if (processMemSetMemCpyDependence(M, MDep))
+  if (DepInfo.isClobber()) {
+    if (MemSetInst *MDep = dyn_cast<MemSetInst>(DepInfo.getInst())) {
+      if (auto *MS = processMemSetMemCpyDependence(M, MDep)) {
+        BBI = MS->getIterator();
         return true;
+      }
+    }
+  }
 
   // The optimizations after this point require the memcpy size.
   ConstantInt *CopySize = dyn_cast<ConstantInt>(M->getLength());
   if (!CopySize) return false;
 
   // There are four possible optimizations we can do for memcpy:
   //   a) memcpy-memcpy xform which exposes redundance for DSE.
   //   b) call-memcpy xform for return slot optimization.
if (DepInfo.isClobber()) {
     }
   }
 
   MemoryLocation SrcLoc = MemoryLocation::getForSource(M);
   MemDepResult SrcDepInfo = MD->getPointerDependencyFrom(
       SrcLoc, true, M->getIterator(), M->getParent());
 
   if (SrcDepInfo.isClobber()) {
-    if (MemCpyInst *MDep = dyn_cast<MemCpyInst>(SrcDepInfo.getInst()))
-      return processMemCpyMemCpyDependence(M, MDep);
+    if (MemCpyInst *MDep = dyn_cast<MemCpyInst>(SrcDepInfo.getInst())) {
+      if (auto *MC = processMemCpyMemCpyDependence(M, MDep)) {
+        BBI = MC->getIterator();
+        return true;
+      }
+      return false;
+    }
   } else if (SrcDepInfo.isDef()) {
     if (hasUndefContents(SrcDepInfo.getInst(), CopySize)) {
       MD->removeInstruction(M);
       M->eraseFromParent();
       ++NumMemCpyInstr;
       return true;
     }
   }
 
   if (SrcDepInfo.isClobber())
     if (MemSetInst *MDep = dyn_cast<MemSetInst>(SrcDepInfo.getInst()))
-      if (performMemCpyToMemSetOptzn(M, MDep)) {
+      if (auto *MS = performMemCpyToMemSetOptzn(M, MDep)) {
         MD->removeInstruction(M);
         M->eraseFromParent();
         ++NumCpyToSet;
+        BBI = MS->getIterator();
         return true;
       }
 
   return false;
 }
 
 /// Transforms memmove calls to memcpy calls when the src/dst are guaranteed
 /// not to alias.
-bool MemCpyOptPass::processMemMove(MemMoveInst *M) {
+/// If changes are made, return true and set BBI to the next instruction to
+/// visit.
+bool MemCpyOptPass::processMemMove(MemMoveInst *M, BasicBlock::iterator &BBI) {
   AliasAnalysis &AA = LookupAliasAnalysis();
 
   if (!TLI->has(LibFunc_memmove))
     return false;
 
   // See if the pointers alias.
   if (!AA.isNoAlias(MemoryLocation::getForDest(M),
                     MemoryLocation::getForSource(M)))
bool MemCpyOptPass::processMemMove(MemMoveInst *M, BasicBlock::iterator &BBI) {
   M->setCalledFunction(Intrinsic::getDeclaration(M->getModule(),
                                                  Intrinsic::memcpy, ArgTys));
 
   // MemDep may have over conservative information about this instruction, just
   // conservatively flush it from the cache.
   MD->removeInstruction(M);
 
   ++NumMoveToCpy;
+  BBI = M->getIterator();
   return true;
 }
 
 /// This is called on every byval argument in call sites.
 bool MemCpyOptPass::processByValArgument(CallBase &CB, unsigned ArgNo) {
   const DataLayout &DL = CB.getCaller()->getParent()->getDataLayout();
   // Find out what feeds this byval argument.
   Value *ByValArg = CB.getArgOperand(ArgNo);
for (BasicBlock &BB : F) {
     // has itself as a predecessor).
     if (!DT.isReachableFromEntry(&BB))
       continue;
 
     for (BasicBlock::iterator BI = BB.begin(), BE = BB.end(); BI != BE;) {
         // Avoid invalidating the iterator.
       Instruction *I = &*BI++;
 
-      bool RepeatInstruction = false;
-
       if (StoreInst *SI = dyn_cast<StoreInst>(I))
         MadeChange |= processStore(SI, BI);
       else if (MemSetInst *M = dyn_cast<MemSetInst>(I))
-        RepeatInstruction = processMemSet(M, BI);
+        MadeChange = processMemSet(M, BI);
       else if (MemCpyInst *M = dyn_cast<MemCpyInst>(I))
-        RepeatInstruction = processMemCpy(M, BI);
+        MadeChange = processMemCpy(M, BI);
       else if (MemMoveInst *M = dyn_cast<MemMoveInst>(I))
-        RepeatInstruction = processMemMove(M);
+        MadeChange = processMemMove(M, BI);
       else if (auto *CB = dyn_cast<CallBase>(I)) {
         for (unsigned i = 0, e = CB->arg_size(); i != e; ++i)
           if (CB->isByValArgument(i))
             MadeChange |= processByValArgument(*CB, i);
       }
-
-      // Reprocess the instruction if desired.
-      if (RepeatInstruction) {
-        if (BI != BB.begin())
-          --BI;
-        MadeChange = true;
-      }
     }
   }
 
   return MadeChange;
 }
 
 PreservedAnalyses MemCpyOptPass::run(Function &F, FunctionAnalysisManager &AM) {
   auto &MD = AM.getResult<MemoryDependenceAnalysis>(F);