This is an archive of the discontinued LLVM Phabricator instance.

[DSE] Lift post-dominance for objs not accessible in caller.
ClosedPublic

Authored by fhahn on Apr 8 2020, 8:29 AM.

Details

Summary

We can eliminate MemoryDefs of objects not accessible after the function
returns (e.g. alloca), if there are no reads between the MemoryDef and
any function exits. We can stop traversing paths that completely
overwrite the memory location of the MemoryDef.

This patch was split off D73763.

Diff Detail

Event Timeline

fhahn created this revision.Apr 8 2020, 8:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2020, 8:29 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
asbirlea added inline comments.Apr 8 2020, 2:16 PM
llvm/test/Transforms/DeadStoreElimination/MSSA/memset-missing-debugloc.ll
39 ↗(On Diff #256031)

I don't fully understand the need for this test change, could you please clarify?

fhahn updated this revision to Diff 256132.Apr 8 2020, 3:56 PM

Remove unnecessary test changes from memset-missing-debugloc.ll

fhahn marked an inline comment as done.Apr 8 2020, 3:59 PM
fhahn added inline comments.
llvm/test/Transforms/DeadStoreElimination/MSSA/memset-missing-debugloc.ll
39 ↗(On Diff #256031)

They are not required for this patch. I think they are supposed to be part of a different patch ( eliminating stores that are never read) and slipped in during a rebase.

Without changes, the function in the file never reads the stores/memset to the alloca and they can be completely removed. But that's not related to the patch and I've removed those changes.

Thanks for this!

Just a few nits. Idea and implementation looks fine to me otherwise

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

s/the caller$/the caller after/

1541

nit: can this just be nested in the above if, rather than repeating !CapturesBeforeRet?

llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-multipath.ll
139

s/because the it/because it/ (similar below)

217

world's smallest ₙᵢₜ: please remove the extra space before 'first'

fhahn updated this revision to Diff 257043.Apr 13 2020, 11:36 AM
fhahn marked 3 inline comments as done.

Thanks for this!

Just a few nits. Idea and implementation looks fine to me otherwise

Should be addressed, thanks!

LGTM. If no one else comments within a day or so, feel free to land.

Thanks again!

This revision is now accepted and ready to land.Apr 13 2020, 1:20 PM
asbirlea accepted this revision.Apr 13 2020, 2:01 PM

LGTM.

This revision was automatically updated to reflect the committed changes.