This is an archive of the discontinued LLVM Phabricator instance.

[DSE] Add stat for remaining stores after DSE.
ClosedPublic

Authored by fhahn on Apr 24 2020, 2:30 PM.

Details

Summary

Using the existing NumFastStores statistic can be misleading when
comparing the impact of DSE patches.

For example, consider the case where a store gets removed from a
function before it is inlined into another function. A less
powerful DSE might only remove the store from functions it has
been inlined into, which will result in more stores being removed, but
no difference in the actual number of stores after DSE.

The new stat provides the absolute number of stores surviving after
DSE.

Diff Detail

Event Timeline

fhahn created this revision.Apr 24 2020, 2:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2020, 2:30 PM
asbirlea accepted this revision.Apr 24 2020, 5:47 PM

I think generally, for accurate stats on how DSE is doing, it would be good to have tests that just run DSE, vs -O3. But this is a fair statistic to have for benchmarks where this is not possible.

This revision is now accepted and ready to land.Apr 24 2020, 5:47 PM
asbirlea added inline comments.Apr 24 2020, 5:50 PM
llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
1984

Nit: for the MemorySSA case, this could walk def chains; but perhaps we don't care much about performance when getting stats?

fhahn marked an inline comment as done.Apr 25 2020, 8:22 AM

I think generally, for accurate stats on how DSE is doing, it would be good to have tests that just run DSE, vs -O3. But this is a fair statistic to have for benchmarks where this is not possible.

I am not sure if I follow entirely. Are you suggesting to run DSE and compare the total number of stores after compiling to -O3 for benchmarks from test-suite?

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

I've additionally guarded the loop by AreStatisticsEnabled() and kept the loop as is for now. I think it is slightly more beneficial to ensure we comparing the same thing between both versions in a straight-forward way.

This revision was automatically updated to reflect the committed changes.