[PATCH] D135574: [MemCpyOpt] Update code comments for processMemSetMemCpyDependence. NFC

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1279-1282
   Value *Ule = Builder.CreateICmpULE(DestSize, SrcSize);
   Value *SizeDiff = Builder.CreateSub(DestSize, SrcSize);
   Value *MemsetLen = Builder.CreateSelect(
       Ule, ConstantInt::getNullValue(DestSize->getType()), SizeDiff);
----------------
Side note: This can be replaced by an usub_sat. I guess it would be safe to do that directly here instead of letting InstCombine detect the pattern later?


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1293
          "MemCpy must be a MemoryDef");
-  // The new memset is inserted after the memcpy, but it is known that its
-  // defining access is the memset about to be removed which immediately
-  // precedes the memcpy.
+  // 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.
----------------
@fhahn : I think you added the MSSA updating here. But the code comments did not match up with the actual movement of the memset. But I think the actual MSSA update seem to match the implementation, so it was only code comment that was wrong. Right?


And as I hinted elsewhere, maybe we want this transform to move the memset below the memcpy. But that seem to make the MSSA update a bit trickier. I kind of assumed that one could use MSSAU->moveAfter/moveBefore in a situation like this? But maybe that require that the memset instruction only is moved and not replaced (as we need to adjust the arguments here) in some way?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135574/new/

https://reviews.llvm.org/D135574