[PATCH] D82204: [DSE,MSSA] Treat `store 0` after calloc as noop stores.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/calloc-store.ll:113
+  store i8 0, i8* %p.2, align 1
+  ret i8* %p
+}
----------------
asbirlea wrote:
> fhahn wrote:
> > asbirlea wrote:
> > > Negative test: add before ret and check it does not get eliminated:
> > > ```
> > > %p.3.2 = getelementptr i8, i8* %p, i32 3
> > >  store i8 0, i8* %p.3.2, align 1
> > > ```
> > Done, I've added it as a separate test. Currently only the last store is left over, but as it is a store of 0, it could also be eliminated. We currently don't do that, because of the order in which we perform the eliminations. 
> I was suggesting a test where the store would intentionally not be eliminated (negative test), and this seems correct below. There is a store of 5, then the store of 0 should not be eliminated because it would overwrite the 5.
> Add a check that the store of value 5 is also there?
> 
> If you mean the store is the last before ret, and it should be eliminated because of *that*, then add another method call afterwards to defeat it. This way, we'll have a unit test to check that DSE works correctly on the particular feature added in this patch, the pattern:
> ```
> calloc
> store non-zero value
> store zero is not eliminated (i.e. clobbering correctly find the non-zero store, not the calloc)
> ```
> 
> Does this make sense?
> I was suggesting a test where the store would intentionally not be eliminated (negative test), and this seems correct below. There is a store of 5, then the store of 0 should not be eliminated because it would overwrite the 5.
> Add a check that the store of value 5 is also there?
I might be missing something, but `store i8 5, i8* %p.3` in @test11 is eliminated because it gets overwritten by the last store ` store i8 0, i8* %p.3.2`. The check lines ensure that it is gone.


> 
> If you mean the store is the last before ret, and it should be eliminated because of *that*, then add another method call afterwards to defeat it.

We are already returning the pointer returned from `calloc`, which should be enough to ensure that non-zero stores cannot be removed because they are not read at the end of the function I think.

 This way, we'll have a unit test to check that DSE works correctly on the particular feature added in this patch, the pattern:
> 
> calloc
> store non-zero value
> store zero is not eliminated (i.e. clobbering correctly find the non-zero store, not the calloc)
> Does this make sense?

IIUC in the scenario above (which is should be reflected in @test11) we could eliminate both stores. The non-zero value is overwritten by 0 before it is read or the function returns, so `calloc` would already ensure the location is zero.

I'll add a test case with a read between the non-zero and zero store. This way, neither can be eliminated.

Does that make sense?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82204