[PATCH] D111727: [DSE] Eliminates redundant store of an exisiting value (PR16520)

Mon Oct 25 03:29:45 PDT 2021

nikic added inline comments.

================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1962
+      if (!MaybeUpperLoc || !DefInst->isIdenticalTo(UpperInst) ||
+          isReadClobber(*MaybeUpperLoc, DefInst))
+        continue;
----------------
yurai007 wrote:
> yurai007 wrote:
> > nikic wrote:
> > > Actually, why do we need this isReadClobber() check? I don't think we care whether the instruction also reads the value, removing the redundant store is still fine. This would allow us to catch the `@pr50339` case as well.
> > It's redundant and can be removed. I thought it was needed to ensure correctness for cases with intermediate clobbering (like tests3/test4). I will get rid of check and adjust tests.
> Ok. Actually we cannot simply get rid of it. Now I see why it is needed. It's for self-reads like:
> 
> call void @llvm.memmove.p0i8.p0i8.i64(i8* %P, i8* %Q, i64 12, i1 false)
> call void @llvm.memmove.p0i8.p0i8.i64(i8* %P, i8* %Q, i64 12, i1 false)
> 
> Without check memmove is eliminated which is wrong when P and Q alias.
Oh right. It's okay to ignore the clobber for memcpy, but not for memmove. Let's leave it as-is for now.

This probably also prevents removing `strcat(x, y); strcat(x, y);`(with x, y noalias) as the second one will read `x` as well to find the insertion point. Might be worth adding a test for that case as well.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111727