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/lib/Transforms/Scalar/MemCpyOptimizer.cpp

 bool MemCpyOptPass::processMemCpy(MemCpyInst *M) {
   // 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()) {
     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);