[PATCH] D82588: [DSE] Look through memory PHI arguments when removing noop stores in MSSA.

Florian Hahn via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Sep 30 10:44:31 PDT 2020


fhahn added inline comments.


================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/simple-todo.ll:8
+; Remove both redundant stores if loaded value is in another block inside a loop.
+define i32 @test47(i1 %c, i32* %p, i32 %i) {
+; CHECK-LABEL: @test47(
----------------
zoecarver wrote:
> fhahn wrote:
> > IIUC we should now handle the original case correctly? Can you move it to `simple.ll` and perhaps add the additional case to `noop-stores.ll`?
> Original case? Do you mean `test31`? I've added that to `noop-stores.ll`. Also, what do you mean by the additional case? `test47`? 
> 
> `test47` we don't yet support but it's a pretty simple fix, I think we can just check if the def is a `StoreInst` and run it through `storeIsNoop` and if that returns true we can continue instead of returning false. 
> Original case? Do you mean test31? I've added that to noop-stores.ll.
 that's fine, I missed that.

> test47 we don't yet support but it's a pretty simple fix, I think we can just check if the def is a StoreInst and run it through storeIsNoop and if that returns true we can continue instead of returning false.

yes I meant test47. `simple-todo.ll` only contains cases from the original `simple.ll` that MSSA-backed DSE did not support. So it would be best to add new test cases to `noop-stores.ll` and remove `simple-todo.ll` once all test cases have been moved out of there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82588



More information about the llvm-commits mailing list