Correctly report modified status for MemCpyOptimizer

[serge-sans-paille]

Summary

Related to https://reviews.llvm.org/D80916

[foad]

Seems obviously correct - but slightly unfortunate that this will cause iterateOnFunction to go back and reprocess a prior instruction.

[serge-sans-paille]

Do you think it's worth passing the iterator as an extra argument?

[serge-sans-paille]

Avoid re-processing the optimized call.

[foad]

Sorry for the late review, but I think the code is getting really confusing now: processMemCpy returns a "repeat instruction" flag, but sometimes returns true, but also increments BI so that the instruction is not repeated at al???

How about changing all of processMemSet/Cpy/Move so that they return a simple "changed" flag, and set BI to the next instruction to be processed (i.e. either the current instruction if we want to repeat it, otherwise the next instruction).

[serge-sans-paille]

In D81229#2082217, @foad wrote:

Sorry for the late review, but I think the code is getting really confusing now: processMemCpy returns a "repeat instruction" flag, but sometimes returns true, but also increments BI so that the instruction is not repeated at al???

How about changing all of processMemSet/Cpy/Move so that they return a simple "changed" flag, and set BI to the next instruction to be processed (i.e. either the current instruction if we want to repeat it, otherwise the next instruction).

Well, I landed the patch... The extra iterator is also passed for other processMem* functions, in order to adjust current iterator, so I used it in processMemCpy. I agree it's non-ideal, but at least it's consistent ;-)

[foad]

Well, I landed the patch...

Yeah but post-commit review is still a real thing! As for cleaning it up, how about D81540?

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

bool runImpl(Function &F, MemoryDependenceResults *MD_,
                std::function<AliasAnalysis &()> LookupAliasAnalysis_,
                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);
+  bool processMemCpy(MemCpyInst *M, BasicBlock::iterator &BBI);
   bool processMemMove(MemMoveInst *M);
   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);
   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

bool MemCpyOptPass::performMemCpyToMemSetOptzn(MemCpyInst *MemCpy,
   return true;
 }
 
 /// 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.
-bool MemCpyOptPass::processMemCpy(MemCpyInst *M) {
+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 false;
+    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 (Value *ByteVal = isBytewiseValue(GV->getInitializer(),
                                            M->getModule()->getDataLayout())) {
         IRBuilder<> Builder(M);
for (BasicBlock::iterator BI = BB.begin(), BE = BB.end(); BI != BE;) {
 
       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);
       else if (MemCpyInst *M = dyn_cast<MemCpyInst>(I))
-        RepeatInstruction = processMemCpy(M);
+        RepeatInstruction = processMemCpy(M, BI);
       else if (MemMoveInst *M = dyn_cast<MemMoveInst>(I))
         RepeatInstruction = processMemMove(M);
       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);
       }