[llvm] a5c3b57 - [MemCpyOpt] Work around PR54682

[Nikita Popov via llvm-commits]

Author: Nikita Popov
Date: 2022-04-04T10:19:51+02:00
New Revision: a5c3b5748c1176c11bdc041271ead5392295742d

URL: https://github.com/llvm/llvm-project/commit/a5c3b5748c1176c11bdc041271ead5392295742d
DIFF: https://github.com/llvm/llvm-project/commit/a5c3b5748c1176c11bdc041271ead5392295742d.diff

LOG: [MemCpyOpt] Work around PR54682

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. On CTMark, this patch 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.

Differential Revision: https://reviews.llvm.org/D122911

Added: 
    

Modified: 
    llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
    llvm/test/Transforms/MemCpyOpt/pr54682.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp b/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
index f886fac16780b..6fa998bf134fe 100644
--- a/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
+++ b/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
@@ -1427,7 +1427,8 @@ bool MemCpyOptPass::processMemCpy(MemCpyInst *M, BasicBlock::iterator &BBI) {
       }
 
   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);

diff  --git a/llvm/test/Transforms/MemCpyOpt/pr54682.ll b/llvm/test/Transforms/MemCpyOpt/pr54682.ll
index 868ed7f9de479..2f18382636f8b 100644
--- a/llvm/test/Transforms/MemCpyOpt/pr54682.ll
+++ b/llvm/test/Transforms/MemCpyOpt/pr54682.ll
@@ -1,7 +1,8 @@
 ; 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(
@@ -17,6 +18,7 @@ define void @test(i1 %c, i8* nocapture noundef readonly %path, i8* noundef write
 ; 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: