[PATCH] D124078: [MemCpyOpt] Avoid infinite loop in processMemSetMemCpyDependence (PR54983)

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1298
           Builder.CreatePointerCast(Dest, Builder.getInt8PtrTy(DestAS)),
           SrcSize),
       MemSet->getOperand(1), MemsetLen, Alignment);
----------------
fhahn wrote:
> I think the underlying issue here is that the builder folds this GEP to `Dest` is `SrcSize` is poison. This in turn leads to the new MemSet to `must-alias` the original memcpy, causing the cycle IIUC. Could we break the cycle here by bailing out if the pointer folds to `Dest` as more general solution and leave the cleanup of the cases where the size simplifies to 0 to instcombine?
In this case, yes:
```
  %1 = icmp ule i64 %len, poison
  %2 = sub i64 %len, poison
  %3 = select i1 %1, i64 0, i64 %2
  %4 = icmp ule i64 %3, poison
  %5 = sub i64 %3, poison
  %6 = select i1 %4, i64 0, i64 %5
  call void @llvm.memset.p0.i64(ptr align 1 inttoptr (i64 -1 to ptr), i8 0, i64 %6, i1 false)
  call void @llvm.memcpy.p0.p0.i64(ptr inttoptr (i64 -1 to ptr), ptr null, i64 poison, i1 false)
  ret void
```
But not in the other case:
```
  %size = shl i64 0, 0
  %1 = icmp ule i64 1, %size
  %2 = sub i64 1, %size
  %3 = select i1 %1, i64 0, i64 %2
  %4 = getelementptr i8, ptr %p, i64 %size
  %5 = icmp ule i64 %3, %size
  %6 = sub i64 %3, %size
  %7 = select i1 %5, i64 0, i64 %6
  %8 = getelementptr i8, ptr %p, i64 %size
  call void @llvm.memset.p0.i64(ptr align 1 %8, i8 0, i64 %7, i1 false)
  call void @llvm.memcpy.p0.p0.i64(ptr %p, ptr %p2, i64 %size, i1 false)
  ret void
```
Here the pointers aren't directly equal, but AA can determine they're equal.

So just checking that we get back the old Dest wouldn't fully avoid the issue.


================
Comment at: llvm/test/Transforms/MemCpyOpt/memcpy-zero-size.ll:34
+  call void @llvm.memset.p0.i64(ptr inttoptr (i64 -1 to ptr), i8 0, i64 %len, i1 false)
+  call void @llvm.memcpy.p0.p0.i64(ptr inttoptr (i64 -1 to ptr), ptr null, i64 poison, i1 false)
+  ret void
----------------
fhahn wrote:
> might be good to use pointer arguments passed to the function here, instead of `null`/`inttoptr` so the `poison` is the only issue here 
I've replaced the null, but the inttoptr is important for the IRBuilder constant folding.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124078/new/

https://reviews.llvm.org/D124078