Page MenuHomePhabricator

[LICM & MemorySSA] Don't sink/hoist stores in the presence of ordered loads.

Authored by asbirlea on Mar 19 2019, 2:41 PM.



Before this patch, if any Use existed in the loop, with a defining
access in the loop, we conservatively decide to not move the store.
What this approach was missing, is that ordered loads are not Uses, they're Defs
in MemorySSA. So, even when the clobbering walker does not find that
volatile load to interfere, we still cannot hoist a store past a
volatile load.
Resolves PR41140.

Diff Detail


Event Timeline

asbirlea created this revision.Mar 19 2019, 2:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2019, 2:41 PM

Thanks for this!

1196 ↗(On Diff #191397)

Is this bit relevant to PR41140? If not, please commit separately.

1213 ↗(On Diff #191397)

nit: please fold the if into this, e.g. else if (const auto *MD = dyn_cast<MemoryDef>(&MA))

1215 ↗(On Diff #191397)

please clang-format

1216 ↗(On Diff #191397)

Should this be an assertion? Loads are only Defs if they're volatile or have >= some strength of atomic ordering

1224 ↗(On Diff #191397)

nit: please fold this into the return: return MSSA->isLiveOnEntryDef(Source) || !CurLoop->contains(...);

asbirlea updated this revision to Diff 191526.Mar 20 2019, 10:18 AM
asbirlea marked 5 inline comments as done.

Address comments.

asbirlea marked an inline comment as done.Mar 20 2019, 10:18 AM
asbirlea added inline comments.
1196 ↗(On Diff #191397)

This was in the previous code, and it's needed. I just refactored the code a bit, and moved this check before calling getClobbering to get an early exit.
Also inverted the OptCap check in the latest update to decrease indent.

asbirlea marked an inline comment as done.Mar 20 2019, 10:19 AM

LGTM; thanks!

This revision is now accepted and ready to land.Mar 20 2019, 11:10 AM
This revision was automatically updated to reflect the committed changes.