[LoopIdiom] Fix bailout for aliasing in memcpy transform.

[efriedma]

Summary

Commit dd5991cc modified the aliasing checks here to allow transforming a memcpy where the source and destination point into the same object. However, the change accidentally made the code skip the alias check for other operations in the loop.

Instead of completely skipping the alias check, just skip the check for whether the memcpy aliases itself.

[yurai007]

Thanks for fixing such nasty bug. Now checking whether loop access load is not only correct but also more straightforward. Maybe it would be good to receive more comments from others but since I cannot find anything wrong with this change, it's LGTM.

llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp

bool LoopIdiomRecognize::processLoopStoreOfLoopLoad(
 
   // For a memcpy, we have to make sure that the input array is not being
   // mutated by the loop.
   Value *LoadBasePtr = Expander.expandCodeFor(
       LdStart, Builder.getInt8PtrTy(LdAS), Preheader->getTerminator());
 
   // If the store is a memcpy instruction, we must check if it will write to
   // the load memory locations. So remove it from the ignored stores.
-  if (IsMemCpy)
-    IgnoredInsts.erase(TheStore);
   MemmoveVerifier Verifier(*LoadBasePtr, *StoreBasePtr, *DL);
+  if (IsMemCpy && !Verifier.IsSameObject)
+    IgnoredInsts.erase(TheStore);
   if (mayLoopAccessLocation(LoadBasePtr, ModRefInfo::Mod, CurLoop, BECount,
                             StoreSizeSCEV, *AA, IgnoredInsts)) {
-    if (!IsMemCpy) {
-      ORE.emit([&]() {
-        return OptimizationRemarkMissed(DEBUG_TYPE, "LoopMayAccessLoad",
-                                        TheLoad)
-               << ore::NV("Inst", InstRemark) << " in "
-               << ore::NV("Function", TheStore->getFunction())
-               << " function will not be hoisted: "
-               << ore::NV("Reason", "The loop may access load location");
-      });
-      return Changed;
-    }
-    // At this point loop may access load only for memcpy in same underlying
-    // object. If that's not the case bail out.
-    if (!Verifier.IsSameObject)
-      return Changed;
-  }
+      return OptimizationRemarkMissed(DEBUG_TYPE, "LoopMayAccessLoad", TheLoad)
 
   bool UseMemMove = IsMemCpy ? Verifier.IsSameObject : LoopAccessStore;
   if (UseMemMove)
     if (!Verifier.loadAndStoreMayFormMemmove(StoreSize, IsNegStride, *TheLoad,
                                              IsMemCpy))
       return Changed;
 
   if (avoidLIRForMultiBlockLoop())

llvm/test/Transforms/LoopIdiom/basic.ll

for.body:                                      ; preds = %entry, %for.body
   %idxprom = zext i32 %index to i64
   %arrayidx2 = getelementptr inbounds i32, i32* %p, i64 %idxprom
   store i32 %1, i32* %arrayidx2, align 4
   %add = add nsw i32 %1, %sum
   %cmp = icmp sgt i32 %index, 1
   br i1 %cmp, label %for.body, label %for.cond.cleanup
 }
 
+; Do not form memmove when there's an aliasing operation, even
+; if the memcpy source and destination are in the same object.
+define void @do_not_form_memmove8(i64* %p) {
+; CHECK-LABEL: @do_not_form_memmove8(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[P2:%.*]] = getelementptr inbounds i64, i64* [[P:%.*]], i64 1000
+; CHECK-NEXT:    br label [[LOOP:%.*]]
+; CHECK:       exit:
+; CHECK-NEXT:    ret void
+; CHECK:       loop:
+; CHECK-NEXT:    [[X4:%.*]] = phi i32 [ 0, [[ENTRY:%.*]] ], [ [[X13:%.*]], [[LOOP]] ]
+; CHECK-NEXT:    [[X5:%.*]] = zext i32 [[X4]] to i64
+; CHECK-NEXT:    [[X7:%.*]] = getelementptr inbounds i64, i64* [[P2]], i64 [[X5]]
+; CHECK-NEXT:    [[X8:%.*]] = bitcast i64* [[X7]] to i8*
+; CHECK-NEXT:    store i64 1, i64* [[X7]], align 4
+; CHECK-NEXT:    [[X11:%.*]] = getelementptr inbounds i64, i64* [[P]], i64 [[X5]]
+; CHECK-NEXT:    [[X12:%.*]] = bitcast i64* [[X11]] to i8*
+; CHECK-NEXT:    tail call void @llvm.memcpy.p0i8.p0i8.i64(i8* [[X12]], i8* [[X8]], i64 8, i1 false)
+; CHECK-NEXT:    [[X13]] = add i32 [[X4]], 1
+; CHECK-NEXT:    [[X14:%.*]] = icmp eq i32 [[X13]], 44
+; CHECK-NEXT:    br i1 [[X14]], label [[EXIT:%.*]], label [[LOOP]]
+;
+entry:
+  %p2 = getelementptr inbounds i64, i64* %p, i64 1000
+  br label %loop
+
+exit:
+  ret void
+
+loop:
+  %x4 = phi i32 [ 0, %entry ], [ %x13, %loop ]
+  %x5 = zext i32 %x4 to i64
+  %x7 = getelementptr inbounds i64, i64* %p2, i64 %x5
+  %x8 = bitcast i64* %x7 to i8*
+  store i64 1, i64* %x7, align 4
+  %x11 = getelementptr inbounds i64, i64* %p, i64 %x5
+  %x12 = bitcast i64* %x11 to i8*
+  tail call void @llvm.memcpy.p0i8.p0i8.i64(i8* %x12, i8* %x8, i64 8, i1 false)
+  %x13 = add i32 %x4, 1
+  %x14 = icmp eq i32 %x13, 44
+  br i1 %x14, label %exit, label %loop
+}
+
 ;; Memcpy formation is still preferred over memmove.
 define void @prefer_memcpy_over_memmove(i8* noalias %Src, i8* noalias %Dest, i64 %Size) {
 ; CHECK-LABEL: @prefer_memcpy_over_memmove(
 ; CHECK-NEXT:  bb.nph:
 ; CHECK-NEXT:    [[SCEVGEP:%.*]] = getelementptr i8, i8* [[SRC:%.*]], i64 42
 ; CHECK-NEXT:    call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 1 [[DEST:%.*]], i8* align 1 [[SCEVGEP]], i64 [[SIZE:%.*]], i1 false)
 ; CHECK-NEXT:    br label [[FOR_BODY:%.*]]
 ; CHECK:       for.body: