[PATCH] D41400: [MemCpyOpt] Perform call slot optimizations through GEPs

[Björn Steinbrink via Phabricator via llvm-commits]

dotdash added inline comments.


================
Comment at: lib/Transforms/Scalar/MemCpyOptimizer.cpp:836
+  // The store to dest may never happen if the call can throw.
+  if (C->mayThrow() && !isa<AllocaInst>(cpyDest))
+    return false;
----------------
efriedma wrote:
> You probably want to clarify why an alloca is special here.
> 
> Does it matter if cpyDest could be accessed from another thread?
> 
> Do you need to check whether any calls between "C" and "cpy" could throw?
> You probably want to clarify why an alloca is special here.

I basically just kept the logic as it was here, but looks like that comment is a bit outdated, and I already made call slot opt a bit too aggressive back when I introduced handling of arguments other than sret ones.

> Do you need to check whether any calls between "C" and "cpy" could throw?

I guess so, already to properly handle arguments that are merely dereferenceable and not sret, to avoid stores from showing up that should not be visible after a throw. Would a check for postdominance be sufficient here?

> Does it matter if cpyDest could be accessed from another thread?

I'm not completely sure, but I'd say that for multithreaded code to behave correctly here, one would need a memory barrier or something similar here anyway, which should break the dependency between the call and the memcpy, so we should not end up here in the first place. Am I missing something here?


================
Comment at: lib/Transforms/Scalar/MemCpyOptimizer.cpp:848
+    APInt APOffset(DL.getPointerSizeInBits(GEP->getPointerAddressSpace()), 0);
+    if (GEP->accumulateConstantOffset(DL, APOffset) && !Offset.isNegative()) {
+      auto Offset = APOffset.getSExtValue();
----------------
efriedma wrote:
> Maybe this code belongs in getPointerDereferenceableBytes, rather than here?
I had some problems where moving that code broke the original user of getPointerDereferenceableBytes. I can try to see if I can fix that.


================
Comment at: lib/Transforms/Scalar/MemCpyOptimizer.cpp:915
+      if (auto G = dyn_cast<GetElementPtrInst>(cpyDest)) {
+        auto P = G->getPointerOperand();
+        auto PI = dyn_cast<Instruction>(P);
----------------
efriedma wrote:
> This check is kind of fragile, in that it assumes the other operands to the GEP are all ConstantInts.
That's a prerequisite to get here, but I see what you mean. I guess I add an explicit `hasAllConstantIndices()` check here?


https://reviews.llvm.org/D41400