[PATCH] Optimize double storing by memset; memcpy (Take two)

[Jim Grosbach]

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:119
@@ -116,2 +118,3 @@
 void PassManagerBuilder::populateModulePassManager(PassManagerBase &MPM) {
+  DEBUG(dbgs() << "top of PassManagerBuilder::populateModulePassManager\n");
   // If all optimizations are disabled, just run the always-inline pass.
----------------
This seems unrelated to the optimization?

================
Comment at: lib/Transforms/Scalar/MemCpyOptimizer.cpp:847
@@ +846,3 @@
+
+  Builder.CreateMemCpy(Dest, Src, DestSize, Align, IsVolatile);
+
----------------
This should copy SrcSize bytes, not DestSize, right?

================
Comment at: lib/Transforms/Scalar/MemCpyOptimizer.cpp:849
@@ +848,3 @@
+
+  Value *NewDest = Builder.CreateGEP(Dest, SrcSize);
+  NewDest->setName("NewDest");
----------------
"NewDest" is a bit vague. Better would be referencing that this is the first byte of the trailing bit we'll be zeroing explicitly.

================
Comment at: lib/Transforms/Scalar/MemCpyOptimizer.cpp:854
@@ +853,3 @@
+  Value* LenDiff = Builder.CreateSub(SrcSize, DestSize);
+  LenDiff->setName("LenDiff");
+  LLVMContext &Ctx = MemCpy->getContext();
----------------
What happens if LenDiff is negative?

================
Comment at: lib/Transforms/Scalar/MemCpyOptimizer.cpp:863
@@ +862,3 @@
+    Builder.CreateMemSet(NewDest, SetValTrunc, NewLen, Align, IsVolatile);
+
+  MD->removeInstruction(MemCpy);
----------------
We should check for the (probably fairly common) case of both SrcLen and DstLen being constants, in which case we don't have to emit instructions to do the arithmetic.

Likewise, we should check if DestLen==SrcLen. That'd be a bit silly in the user's code, but this sort of pattern can often come from autogenerated code that's not smart enough to elide the bzero() in that case. We should just do it for them.

================
Comment at: lib/Transforms/Scalar/MemCpyOptimizer.cpp:868
@@ +867,3 @@
+  MemSet->eraseFromParent();
+  DEBUG(NewMemSet->getParent()->dump());
+  
----------------
The DEBUG() line needs a lot more context about what it's logging. For anyone looking at debug output that's not actively working on this exact optimization, the instruction dump() here is going to be baffling.

================
Comment at: lib/Transforms/Scalar/MemCpyOptimizer.cpp:1075
@@ -1000,2 +1074,3 @@
 bool MemCpyOpt::runOnFunction(Function &F) {
+  DEBUG(dbgs() << "top of MemCpyOpt::runOnFunction\n");
   bool MadeChange = false;
----------------
Seems not related to the optimization? If we want to add good DEBUG() output for these transforms, that's great, but it should be done as a separate patch first.


http://llvm-reviews.chandlerc.com/D498