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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea accepted this revision.
asbirlea added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/calloc-store.ll:113
+  store i8 0, i8* %p.2, align 1
+  ret i8* %p
+}
----------------
fhahn wrote:
> 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?
> 
> 
Yes, that's perfect. You're right, I missed the need for a use inbetween, otherwise the 5 stored also gets eliminated. Thank you for adding this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82204