This is an archive of the discontinued LLVM Phabricator instance.

[DSE] Eliminate zero memset after calloc
ClosedPublic

Authored by xbolva00 on Apr 27 2021, 12:31 PM.

Details

Summary

Solves PR11896

As noted, this can be improved futher (calloc -> malloc) in some cases. But for know, this is the first step.

Diff Detail

Event Timeline

xbolva00 created this revision.Apr 27 2021, 12:31 PM
xbolva00 requested review of this revision.Apr 27 2021, 12:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2021, 12:31 PM

As I worked on this PR, I found a regression where we fail to eliminate stores/memset due to inaccessiblememonly on calloc. (store_zero_after_calloc_inaccessiblememonly & zero_memset_after_calloc_inaccessiblememonly)

Please check https://bugs.llvm.org/show_bug.cgi?id=50143 (but this patch can be reviewed and pushed separately).

nikic added a subscriber: nikic.Apr 27 2021, 1:16 PM

I think it would be good to add a negative test where there is a store to the calloc in the meantime.

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
1783

nit: unnecessary newline.

I think it would be good to add a negative test where there is a store to the calloc in the meantime.

zero_memset_and_store_after_calloc?
store is killed by memset and then we have a calloc with memset.

I think it would be good to add a negative test where there is a store to the calloc in the meantime.

Yes, good idea. Prepared partial_zero_memset_and_store_after_calloc and zero_memset_and_store_with_dyn_index_after_calloc.

xbolva00 updated this revision to Diff 340969.Apr 27 2021, 1:34 PM

New testcases + fixed nit.

xbolva00 updated this revision to Diff 340976.Apr 27 2021, 1:43 PM

Cleanup tests for easier review.

nikic accepted this revision.Apr 27 2021, 2:12 PM

LGTM

llvm/test/Transforms/DeadStoreElimination/noop-stores.ll
372

I'd suggest adding a variant that stores a constant but non-zero value (e.g. i8 1).

It may also be worthwhile to make sure that passing i1 true for the volatile flag will prevent DSE (this should be covered by the isRemovable check in the caller).

This revision is now accepted and ready to land.Apr 27 2021, 2:12 PM
xbolva00 updated this revision to Diff 340984.Apr 27 2021, 2:18 PM

Added suggested tests

xbolva00 marked an inline comment as done.Apr 27 2021, 2:19 PM
xbolva00 added inline comments.
llvm/test/Transforms/DeadStoreElimination/noop-stores.ll
372

Sure, I will add such tests.

This revision was landed with ongoing or failed builds.Apr 27 2021, 6:31 PM
This revision was automatically updated to reflect the committed changes.
xbolva00 marked an inline comment as done.