[PATCH] D150970: (WIP) [MemCpyOpt]remove memcpy on immutable arguments from attributes

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1627
+bool MemCpyOptPass::processImmutArgument(CallBase &CB, unsigned ArgNo) {
+  // ensure passed argument is immutable during call.
+  // FIXME: no write on call base can be guaranteed by ModRefCheck
----------------
nit: Start comments with uppercase letter.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1640
+  // %val3 = select i1 %c, ptr %val1, ptr %val2
+  AllocaInst *AI = dyn_cast<AllocaInst>(ImmutArg->stripPointerCasts());
+  if (!AI)
----------------
nit: `auto *` when using `dyn_cast`.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1661
+  // If the immut argument isn't fed by a memcpy, ignore it.  If it is fed by
+  // a memcpy, see the arg equals to memcpy dest
+  if (!MDep || MDep->isVolatile() ||
----------------
see -> check that


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1663
+  if (!MDep || MDep->isVolatile() ||
+      ImmutArg->stripPointerCasts() != MDep->getDest())
+    return false;
----------------
`ImmutArg->stripPointerCasts()` is same as AI.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1666
+
+  // currently handle only `noalias` attributed argument  or unescaped
+  // The length of the memcpy must be larger or equal to the size of the byval.
----------------
Outdated comment?


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1667
+  // currently handle only `noalias` attributed argument  or unescaped
+  // The length of the memcpy must be larger or equal to the size of the byval.
+  // memcpy size == allocasize
----------------
byval -> alloca?


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1678
+  if ((!MemDepAlign || *MemDepAlign < AllocaAlign) &&
+      getOrEnforceKnownAlignment(MDep->getSource(), MaybeAlign(AllocaAlign), DL,
+                                 &CB, AC, DT) < AllocaAlign)
----------------
Is the explicit `MaybeAlign()` here needed?


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1693
+  // It would be invalid to transform the second memcpy into foo(*b).
+  // Also verify source is not modified by alias.
+  if (writtenBetween(MSSA, BAA, MemoryLocation::getForSource(MDep),
----------------
"not modified by the call" maybe?


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1695
+  if (writtenBetween(MSSA, BAA, MemoryLocation::getForSource(MDep),
+                     MSSA->getMemoryAccess(MDep), MSSA->getMemoryAccess(&CB)) ||
+      writtenBetween(MSSA, BAA, MemoryLocation::getForDest(MDep),
----------------
`MSSA->getMemoryAccess(&CB)` is `CallAccess`.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1697
+      writtenBetween(MSSA, BAA, MemoryLocation::getForDest(MDep),
+                     MSSA->getMemoryAccess(MDep), MSSA->getMemoryAccess(&CB)) ||
+      isModSet(AA->getModRefInfo(&CB, MemoryLocation::getForDest(MDep))))
----------------
This second writtenBetween looks redundant to me. This is already ensured by getClobberingMemoryAccess().


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1698
+                     MSSA->getMemoryAccess(MDep), MSSA->getMemoryAccess(&CB)) ||
+      isModSet(AA->getModRefInfo(&CB, MemoryLocation::getForDest(MDep))))
+    return false;
----------------
Hm, shouldn't this be checking the source? We already know the dest is not modified via readonly noalias.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1708
+    TmpCast = TmpBitCast;
+  }
+
----------------
You can drop the bitcast code, not needed with opaque pointers.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1754
+            MadeChange |= processImmutArgument(*CB, i);
+          }
+        }
----------------
nit: Unnecessary braces.


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

https://reviews.llvm.org/D150970