This is an archive of the discontinued LLVM Phabricator instance.

[MemorySSA] Allow reordering of loads that alias in the presence of volatile loads.
ClosedPublic

Authored by asbirlea on Dec 21 2017, 3:09 PM.

Details

Summary

Make MemorySSA allow reordering of two loads that may alias, when one is volatile.
This makes MemorySSA less conservative and behaving the same as the AliasSetTracker.
For more context, see D16875.

LLVM language reference: "The optimizers must not change the number of volatile operations or change their order of execution relative to other volatile operations. The optimizers may change the order of volatile operations relative to non-volatile operations. This is not Java’s “volatile” and has no cross-thread synchronization behavior."

Diff Detail

Repository
rL LLVM

Event Timeline

asbirlea created this revision.Dec 21 2017, 3:09 PM

Thanks for this! Just a few nitpicks.

I think this is the right way forward, though please wait until tomorrow to land this change, so people have the chance to chime in if they'd like to.

lib/Analysis/MemorySSA.cpp
210 ↗(On Diff #127957)

Since this code is pretty subtle, can we keep a comment here saying something like "Otherwise, volatile doesn't matter here. From the langref: 'optimizers may change the order of volatile operations relative to non-volatile operations.'"?

223 ↗(On Diff #127957)

Can we simplify this to return !(SeqCstUse || MayClobberIsAcquire) (or the !A && !B equivalent; whatever you feel is cleaner)?

257 ↗(On Diff #127957)

Remove these braces, too, please.

This revision is now accepted and ready to land.Dec 21 2017, 3:43 PM
asbirlea updated this revision to Diff 127971.Dec 21 2017, 4:49 PM

Address comments.

Thank you for the review!

asbirlea marked 3 inline comments as done.Dec 21 2017, 5:03 PM
This revision was automatically updated to reflect the committed changes.