[MemCpyOpt] Move GEP during call slot optimization

[nikic]

Summary

When performing a call slot optimization to a GEP dest, it will currently usually not apply, because the GEP is directly before the memcpy and as such does not dominate the call. We should move it above the call if that satisfies the domination requirement. I think that a constant-index GEP is the only useful thing to move here, as otherwise isDereferenceablePointer couldn't look through it anyway. As such I'm not trying to generalize this further.

[nikic]

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

920

I'm wondering if it would make sense to extend the dominates() API to accept a Value* instead of Instruction* as first argument. Handling "arguments, constants and globals dominate everything" seems like something the DominatorTree API should do, not the caller.

[jdoerfert]

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

920

agreed.

[nikic]

Rebase over generalized dominates() API.

[jdoerfert]

LGTM.

[dmajor]

Hi, we have a regression that started with this commit. The problem happens when GEP %43 is moved before the call on line 49: https://paste.mozilla.org/gYqBnKes . Can you see anything obviously wrong with that move?

If not, our repro is quite large and I fear it will take me a very long time to fully understand what went wrong and/or get a runnable reduced test case. At this point I'm not even sure whether this transformation was itself the culprit or merely uncovered a problem elsewhere. Hoping you might be able to quickly spot something and save me lots of time!

[dmajor]

I thiiiink I'm starting to see this a little better, and the problem is that the memcpy clobbers the optimized call slot.

Before performCallSlotOptzn

call func(..., X, ...)
load from X
GEP
store to Y

After performCallSlotOptzn

GEP
call func(..., Y, ...)
load from X
store to Y  ; oops

Am I interpreting that correctly?

[nikic]

@dmajor If the call slot optimization applies, then that load/store pair should also get dropped as part of the optimization. I don't immediately see how we can end up both changing the call argument and not dropping the load/store, if I understand right what is happening in your case.

[dmajor]

In D89623#2361288, @nikic wrote:

@dmajor If the call slot optimization applies, then that load/store pair should also get dropped as part of the optimization. I don't immediately see how we can end up both changing the call argument and not dropping the load/store, if I understand right what is happening in your case.

You're right, I was looking too narrowly at performCallSlotOptzn. The IR is sane after MemCpyOptPass as a whole. Our badness actually happens later in the pipeline, sorry for the noise.

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

bool MemCpyOptPass::performCallSlotOptzn(Instruction *cpyLoad,
   // Check that src isn't captured by the called function since the
   // transformation can cause aliasing issues in that case.
   for (unsigned ArgI = 0, E = C->arg_size(); ArgI != E; ++ArgI)
     if (C->getArgOperand(ArgI) == cpySrc && !C->doesNotCapture(ArgI))
       return false;
 
   // Since we're changing the parameter to the callsite, we need to make sure
   // that what would be the new parameter dominates the callsite.
-  // TODO: Support moving instructions like GEPs upwards.
-  if (Instruction *cpyDestInst = dyn_cast<Instruction>(cpyDest))
-    if (!DT->dominates(cpyDestInst, C))
+  if (!DT->dominates(cpyDest, C)) {
+    // Support moving a constant index GEP before the call.
+    auto *GEP = dyn_cast<GetElementPtrInst>(cpyDest);
+    if (GEP && GEP->hasAllConstantIndices() &&
+        DT->dominates(GEP->getPointerOperand(), C))
+      GEP->moveBefore(C);
+    else
       return false;
+  }
 
   // In addition to knowing that the call does not access src in some
   // unexpected manner, for example via a global, which we deduce from
   // the use analysis, we also need to know that it does not sneakily
   // access dest.  We rely on AA to figure this out for us.
   ModRefInfo MR = AA->getModRefInfo(C, cpyDest, LocationSize::precise(srcSize));
   // If necessary, perform additional analysis.
   if (isModOrRefSet(MR))

llvm/test/Transforms/MemCpyOpt/callslot.ll

;
   ret void
 }
 
 define void @dest_is_gep_requires_movement() {
 ; CHECK-LABEL: @dest_is_gep_requires_movement(
 ; CHECK-NEXT:    [[DEST:%.*]] = alloca [16 x i8], align 1
 ; CHECK-NEXT:    [[SRC:%.*]] = alloca [8 x i8], align 1
 ; CHECK-NEXT:    [[SRC_I8:%.*]] = bitcast [8 x i8]* [[SRC]] to i8*
-; CHECK-NEXT:    call void @accept_ptr(i8* [[SRC_I8]]) [[ATTR3]]
 ; CHECK-NEXT:    [[DEST_I8:%.*]] = getelementptr [16 x i8], [16 x i8]* [[DEST]], i64 0, i64 8
-; CHECK-NEXT:    call void @llvm.memcpy.p0i8.p0i8.i64(i8* [[DEST_I8]], i8* [[SRC_I8]], i64 8, i1 false)
+; CHECK-NEXT:    [[DEST_I81:%.*]] = bitcast i8* [[DEST_I8]] to [8 x i8]*
+; CHECK-NEXT:    [[DEST_I812:%.*]] = bitcast [8 x i8]* [[DEST_I81]] to i8*
+; CHECK-NEXT:    call void @accept_ptr(i8* [[DEST_I812]]) [[ATTR3]]
 ; CHECK-NEXT:    ret void
 ;
   %dest = alloca [16 x i8]
   %src = alloca [8 x i8]
   %src.i8 = bitcast [8 x i8]* %src to i8*
   call void @accept_ptr(i8* %src.i8) nounwind
   %dest.i8 = getelementptr [16 x i8], [16 x i8]* %dest, i64 0, i64 8
   call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dest.i8, i8* %src.i8, i64 8, i1 false)