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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D79391#2033000 <https://reviews.llvm.org/D79391#2033000>, @zoecarver wrote:

> > I usually try to evaluate the impact by comparing the stats for DeadStoreElimination on the test suite with and without the patch. It would probably be good to do this for this patch as well :)
>
> Sounds good. What's the best way to do this? It looks like I can pass the `-stats` option to `opt` but, is there a way to get this behavior across the whole test suit?


Yes, when you build the test-suite, you can set `TEST_SUITE_COLLECT_STATS=On` to collect stats. The final `.json` file after running the test-suite will contain the statistics collected. For more details on how to use the test-suite, see https://llvm.org/docs/TestSuiteGuide.html

I usually use something like

  cmake -G Ninja \
      -DCMAKE_C_COMPILER=/path/to/llvm/build/bin/clang \
      -DCMAKE_CXX_COMPILER=/path/to/llvm/build/bin/clang++ \
      -DCMAKE_C_FLAGS="-O3" -DCMAKE_CXX_FLAGS="-O3 " \
      -DTEST_SUITE_COLLECT_STATS=On \
      /path/to/test-suite
  
  ninja -j 16



================
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);
----------------
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). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79391