[PATCH] D79391: [DSE] Remove noop stores in MSSA.

[Zoe Carver via Phabricator via llvm-commits]

zoecarver marked an inline comment as done.
zoecarver added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1815
+    if (auto *Load = dyn_cast<LoadInst>(SI->getOperand(0))) {
+      if (Load->getPointerOperand() == SI->getOperand(1)) {
+        State.deleteDeadInstruction(SI);
----------------
fhahn wrote:
> zoecarver wrote:
> > fhahn wrote:
> > > zoecarver wrote:
> > > > asbirlea wrote:
> > > > > fhahn wrote:
> > > > > > I think the check is not strict enough at the moment. I think we have to ensure that there are no memory-writes between the load and the store that may alias them.
> > > > > > 
> > > > > > In the simple example below, IIUC the patch would currently incorrectly eliminate  ` store i32 %a, i32* %Q`.
> > > > > > 
> > > > > > ```
> > > > > > define void @test(i32* %Q) {
> > > > > >   %a = load i32, i32* %Q
> > > > > >   store i32 10, i32* %Q
> > > > > >   store i32 %a, i32* %Q
> > > > > >   ret void
> > > > > > }```
> > > > > > 
> > > > > > In terms of MemorySSA, I think we are looking for loads that use the same MemoryDef as `KillingDef`. One simple way to check this would be getting the memoryDef of KillingDef and check if it dominates the load I think.
> > > > > > 
> > > > > +1.
> > > > > Please add a testcase similar to the example fhahn@ gave.
> > > > Will do. I tried the test case and it fails. I'll need to re-write most of this patch (minus the tests). I think @fhahn's solution will work. I probably won't get to this today but I should have time this weekend. 
> > > Awesome, thanks! Feel free to let us know if you have any questions.
> > >  I think we are looking for loads that use the same MemoryDef as KillingDef.
> > I tried playing around with getting the memory access of `Load` but, I'm not sure how it should be the "same" as KillingDef (I don't know a whole lot about MSSA). 
> > 
> > I also tried analyzing `ToCheck` but, I ended up essentially having to implement this check lower down which meant missing some optimizations. 
> I was referring to MemoryDef/MemoryUse in MemorySSA (see http://llvm.org/docs/MemorySSA.html for a bit more details)
> 
> The problem with directly looking at the def-use chains in the IR is that it does not  necessarily include   all memory instructions that might overwrite a the same memory location. MemorySSA provides an 'virtual SSA IR' for memory operations. 
> 
> I think you can make sure that there are no writes may-alias writes in between Load and SI by getting their defining accesses  using MemorySSA (https://llvm.org/doxygen/classllvm_1_1MemoryUseOrDef.html#abc4e1b6cc483cfd57ca75c4ab92bd360). 
Heh, I probably should have spent some time reading the docs before diving right in. I get it now. This is actually way simpler (and probably safer). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79391