This is an archive of the discontinued LLVM Phabricator instance.

[DSE] Remove noop stores after killing stores for a MemoryDef.
ClosedPublic

Authored by fhahn on Oct 18 2020, 10:19 AM.

Details

Summary

Currently we fail to eliminate some noop stores if there is a kill-able
store between the starting def and the load. This is because we
eliminate noop stores first.

In practice it seems like eliminating noop stores after the main
elimination for a def covers slightly more cases.

This patch improves the number of stores slightly in 2 cases for X86 -O3
-flto

Same hash: 235 (filtered out)
Remaining: 2
Metric: dse.NumRedundantStores

Program base patch diff
test-suite...ce/Benchmarks/PAQ8p/paq8p.test 2.00 3.00 50.0%
test-suite...006/453.povray/453.povray.test 18.00 21.00 16.7%

There might be other phase ordering issues, but it appears that they do
not show up in the test-suite/SPEC2000/SPEC2006. We can always tune the
ordering later.

Partly fixes PR47887.

Diff Detail

Event Timeline

fhahn created this revision.Oct 18 2020, 10:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2020, 10:19 AM
fhahn requested review of this revision.Oct 18 2020, 10:19 AM
xbolva00 added inline comments.Oct 26 2020, 8:53 AM
llvm/test/Transforms/DeadStoreElimination/MSSA/noop-stores.ll
291

Update comment?

This LGTM. I wonder if there are any cases where passing the killing memory def in storeIsNoop to storeIsNoop would benefit us. I can't think of one off the top of my head, because it doesn't really matter what order the noop stores come in. But maybe if there was a weird block ordering. I'll see if I can come up with an example.

fhahn updated this revision to Diff 301232.Oct 28 2020, 4:10 AM

This LGTM. I wonder if there are any cases where passing the killing memory def in storeIsNoop to storeIsNoop would benefit us. I can't think of one off the top of my head, because it doesn't really matter what order the noop stores come in. But maybe if there was a weird block ordering. I'll see if I can come up with an example.

Yes there might be some other cases we still miss with this ordering, but so far from the testing I did those did not seem to really show up.

We can always further improve this if additional cases surface.

fhahn added a comment.Oct 28 2020, 4:10 AM
llvm/test/Transforms/DeadStoreElimination/MSSA/noop-stores.ll
291

Thanks, I updated the comment.

fhahn updated this revision to Diff 301562.Oct 29 2020, 4:01 AM

rebase & ping :)

We can always further improve this if additional cases surface.

Sounds good to me.

This patch looks good to me.

We can always further improve this if additional cases surface.

Sounds good to me.

This patch looks good to me.

Click on Accept to formally approve it.

zoecarver accepted this revision.Oct 29 2020, 9:34 AM

Click on Accept to formally approve it.

I wasn't sure if I was allowed to. For example, in libc++ for a while (not anymore), only maintainers were allowed to approve patches.

This revision is now accepted and ready to land.Oct 29 2020, 9:34 AM
asbirlea accepted this revision.Oct 29 2020, 5:37 PM
fhahn added a comment.Oct 30 2020, 2:32 AM

Click on Accept to formally approve it.

I wasn't sure if I was allowed to. For example, in libc++ for a while (not anymore), only maintainers were allowed to approve patches.

Thanks for taking a look! There are no hard rules I am aware of and generally all comments are very welcome!

As for the 'official' approval, I think it depends a lot on the complexity of the patch and if in doubt it is probably best to wait for feedback from a reviewer that already reviewed patches in the area, in addition to other approvals.

This revision was landed with ongoing or failed builds.Oct 30 2020, 2:40 AM
This revision was automatically updated to reflect the committed changes.