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

Mon Oct 25 02:04:26 PDT 2021

yurai007 added inline comments.

================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1962
+      if (!MaybeUpperLoc || !DefInst->isIdenticalTo(UpperInst) ||
+          isReadClobber(*MaybeUpperLoc, DefInst))
+        continue;
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111727