[PATCH] D136822: [InstCombine] Allow memcpys from constant memory to readonly noalias parameters to be elided.

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:91
         // ignore it if we know that the value isn't captured.
-        if (Call->onlyReadsMemory() &&
-            (Call->use_empty() || Call->doesNotCapture(DataOpNo)))
+        bool CapturesArg = !Call->doesNotCapture(DataOpNo);
+        if ((Call->onlyReadsMemory() && (Call->use_empty() || !CapturesArg)) ||
----------------
Nit: It's kind of odd that you negate doesNotCapture here, and then negate it again at both uses.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:93
+        if ((Call->onlyReadsMemory() && (Call->use_empty() || !CapturesArg)) ||
+            (IsArgOperand && Call->onlyReadsMemory(DataOpNo) && !CapturesArg))
           continue;
----------------
Why the `IsArgOperand` check here?


================
Comment at: llvm/test/Transforms/InstCombine/forward-memcpy-to-readonly.ll:4
+;
+; RUN: opt -S -passes=instcombine < %s | FileCheck %s
+
----------------
It looks like the existing tests for this transform are in memcpy-from-global.ll, I'd suggest adding the new test cases there as well. test3-test9 have call-related cases.


================
Comment at: llvm/test/Transforms/InstCombine/forward-memcpy-to-readonly.ll:17
+  call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 4 dereferenceable(4096) %1, ptr noundef nonnull align 4 dereferenceable(4096) @x, i64 4096, i1 false)
+  call void @foo(ptr noundef nonnull nocapture readonly %1) #4
+  ret void
----------------
Drop noundef/nonnull here, as they are not relevant to the transform.

There should be two additional negative tests, where the transform does not occur:
 * Argument is only `readonly`, without `nocapture`.
 * The pointer is also passed to a second argument that is not `readonly`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136822