[PATCH] D122911: [MemCpyOpt] Work around PR54682

[Nikita Popov via Phabricator via llvm-commits]

nikic created this revision.
nikic added reviewers: asbirlea, fhahn.
Herald added subscribers: george.burgess.iv, hiraditya.
Herald added a project: All.
nikic requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

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.


https://reviews.llvm.org/D122911

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


Index: llvm/test/Transforms/MemCpyOpt/pr54682.ll
===================================================================
--- llvm/test/Transforms/MemCpyOpt/pr54682.ll
+++ 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 @@
 ; 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:
Index: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
+++ llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
@@ -1423,7 +1423,8 @@
       }
 
   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);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D122911.419770.patch
Type: text/x-patch
Size: 1652 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220401/c03b09b4/attachment.bin>