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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope created this revision.
bjope added a reviewer: fhahn.
Herald added subscribers: hiraditya, kristof.beyls.
Herald added a project: All.
bjope requested review of this revision.
Herald added a project: LLVM.

Make sure the code comments in processMemSetMemCpyDependence match
with the actual transform. They indicated that the memset being
rewritten was sunk to after a memcpy, while it actually is inserted
just before the memcpy.

Also adding some FIXME comments related to debug info. The new
memset currently inherit debug location from the memcpy rather than
the old memset (which is a bit weird IMO). Maybe that is a good
thing considering that we may end up sinking the memset longer than
needed, such as moving past any dbg instructions that originally
might have been between the memset and memcpy. Such movement is not
really motivated, but afaict it isn't harmful to the end product
as long as we use the incorrect debug location for the new memset.
A totally different idea would be to actually sink the memset down
below the memcpy (to make the memory accesses in increasing address
order). For a target with out-of-order execution that might even be
good since it can proceed with the memset while waiting for the
copying to be performed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135574

Files:
  llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp


Index: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
+++ llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
@@ -1199,12 +1199,14 @@
 /// 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);
+///   memcpy(dst, src, src_size);
 /// \endcode
 bool MemCpyOptPass::processMemSetMemCpyDependence(MemCpyInst *MemCpy,
                                                   MemSetInst *MemSet) {
@@ -1250,8 +1252,21 @@
     if (auto *SrcSizeC = dyn_cast<ConstantInt>(SrcSize))
       Alignment = commonAlignment(DestAlign, SrcSizeC->getZExtValue());
 
+  // SrcSize must be available for the pointer/size calculations. Therefore we
+  // insert the new memset immediately before the MemCpy. An alternative would
+  // be to insert it just after the MemCpy.
+  // FIXME: If we want to insert the new memset above the memcpy, then we might
+  // wannt skip backward past any dbg instructions that are immediate
+  // predecessors to the memcpy. Any such dbg instructions were between the
+  // memset and the memcpy originally, so sinking the memset past them seems a
+  // bit reckless.
   IRBuilder<> Builder(MemCpy);
 
+  // FIXME: For debugability etc. it might be better to use the debug location
+  // of the old memset for the code emitted here rather than using the debug
+  // location from the MemCpy as we do right now.
+  //   Builder.SetCurrentDebugLocation(MemSet->getDebugLoc());
+
   // If the sizes have different types, zext the smaller one.
   if (DestSize->getType() != SrcSize->getType()) {
     if (DestSize->getType()->getIntegerBitWidth() >
@@ -1275,9 +1290,8 @@
 
   assert(isa<MemoryDef>(MSSAU->getMemorySSA()->getMemoryAccess(MemCpy)) &&
          "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.
   auto *LastDef =
       cast<MemoryDef>(MSSAU->getMemorySSA()->getMemoryAccess(MemCpy));
   auto *NewAccess = MSSAU->createMemoryAccessBefore(


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D135574.466479.patch
Type: text/x-patch
Size: 2500 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20221010/7a920848/attachment.bin>