[PATCH] D41400: [MemCpyOpt] Perform call slot optimizations through GEPs
Björn Steinbrink via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 21 06:46:20 PST 2017
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
More information about the llvm-commits
mailing list