[MemCpyOpt] Work around PR54682

[nikic]

Summary

As discussed on https://github.com/llvm/llvm-project/issues/54682, MemorySSA currently has a bug when computing the clobber of calls that access loop-varying locations. I think a "proper" fix for this on the MemorySSA side might be non-trivial, but we can easily work around this in MemCpyOpt:

Currently, MemCpyOpt uses a location-less getClobberingMemoryAccess() call to find a clobber on either the src or dest location, and then refines it for the src and dest clobber. This was intended as an optimization, as the location-less API is cached, while the location-affected APIs are not.

However, I don't think this really makes a difference in practice, because I don't think anything will use the cached clobbers on those calls later anyway. Per http://llvm-compile-time-tracker.com/compare.php?from=cd55e51516f03203f3bf632ff4a65ae7518a8319&to=3a6ff3cb24c5e2fc4b9cd70c80f96bdce6cfa405&stat=instructions the impact seems to be very mildly positive actually.

So I think this is a reasonable way to avoid the problem for now, though MemorySSA should also get a fix.

[fhahn]

LGTM, thanks!

Not sure if it is directly related, but DSE has some code to check whether locations are guaranteed to be loop invariant (or are accessed in the same loop iteration) to deal with AA queries across loop boundaries.

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

if (GV->isConstant() && GV->hasDefinitiveInitializer())
         MSSAU->insertDef(cast<MemoryDef>(NewAccess), /*RenameUses=*/true);
 
         eraseInstruction(M);
         ++NumCpyToSet;
         return true;
       }
 
   MemoryUseOrDef *MA = MSSA->getMemoryAccess(M);
-  MemoryAccess *AnyClobber = MSSA->getWalker()->getClobberingMemoryAccess(MA);
+  // FIXME: Not using getClobberingMemoryAccess() here due to PR54682.
+  MemoryAccess *AnyClobber = MA->getDefiningAccess();
   MemoryLocation DestLoc = MemoryLocation::getForDest(M);
   const MemoryAccess *DestClobber =
       MSSA->getWalker()->getClobberingMemoryAccess(AnyClobber, DestLoc);
 
   // Try to turn a partially redundant memset + memcpy into
   // memcpy + smaller memset.  We don't need the memcpy size for this.
   // The memcpy most post-dom the memset, so limit this to the same basic
   // block. A non-local generalization is likely not worthwhile.

llvm/test/Transforms/MemCpyOpt/pr54682.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -S -memcpyopt < %s | FileCheck %s
 
-; FIXME: This currently gets miscompiled.
+; The memcpy here is *not* dead, because it reads memory written in a previous
+; loop iteration.
 
 define void @test(i1 %c, i8* nocapture noundef readonly %path, i8* noundef writeonly %name) {
 ; CHECK-LABEL: @test(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[TMP:%.*]] = alloca [260 x i8], align 16
 ; CHECK-NEXT:    br label [[WHILE_BODY:%.*]]
 ; CHECK:       while.body:
 ; CHECK-NEXT:    [[IV:%.*]] = phi i64 [ [[IV_NEXT:%.*]], [[WHILE_BODY]] ], [ 259, [[ENTRY:%.*]] ]
 ; CHECK-NEXT:    [[IV_NEXT]] = add nsw i64 [[IV]], -1
 ; CHECK-NEXT:    [[TMP_IV:%.*]] = getelementptr inbounds [260 x i8], [260 x i8]* [[TMP]], i64 0, i64 [[IV]]
 ; CHECK-NEXT:    store i8 42, i8* [[TMP_IV]], align 1
 ; CHECK-NEXT:    br i1 [[C:%.*]], label [[WHILE_BODY]], label [[EXIT:%.*]]
 ; CHECK:       exit:
 ; CHECK-NEXT:    [[TMP_IV_1:%.*]] = getelementptr inbounds i8, i8* [[TMP_IV]], i64 1
 ; CHECK-NEXT:    [[LEN:%.*]] = sub nsw i64 259, [[IV]]
+; CHECK-NEXT:    call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 1 [[NAME:%.*]], i8* nonnull align 1 [[TMP_IV_1]], i64 [[LEN]], i1 false)
 ; CHECK-NEXT:    ret void
 ;
 entry:
   %tmp = alloca [260 x i8], align 16
   br label %while.body
 
 while.body:
   %iv = phi i64 [ %iv.next, %while.body ], [ 259, %entry ]