This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Summary

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!

lib/Transforms/Scalar/LICM.cpp
1196

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

1222

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

1224

please clang-format

1225

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

1227

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.
lib/Transforms/Scalar/LICM.cpp
1196

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.