Page MenuHomePhabricator

[ARM] Be super conservative about atomics
ClosedPublic

Authored by reames on Feb 20 2019, 8:10 PM.

Details

Summary

As requested during review of D57601, be equally conservative for atomic MMOs as for volatile MMOs in all in tree backends. At the moment, all atomic MMOs are also volatile, but I'm about to change that.

I don't really like this patch since I can't test it. I have no idea what "correct" codegen looks like for these targets, so I'm taking "no tests fail" to mean the patch is correct. If any target owner wants to take over their part and write better tests, please feel free.

Diff Detail

Repository
rL LLVM

Event Timeline

reames created this revision.Feb 20 2019, 8:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2019, 8:10 PM

The SystemZ part LGTM, thanks.

The Hexagon changes are OK.

arsenm added a subscriber: arsenm.Feb 21 2019, 6:35 AM

Should all of these use hasOrderedMemoryRef instead?

lib/Target/ARM/ARMLoadStoreOptimizer.cpp
1583–1584 ↗(On Diff #187722)

Is this really any different than hasOrderedMemoryRef?

reames marked an inline comment as done.Feb 21 2019, 9:37 AM
reames added inline comments.
lib/Target/ARM/ARMLoadStoreOptimizer.cpp
1583–1584 ↗(On Diff #187722)

They're not obviously equivalent. I have no idea if they'd achieve the same effect here. If someone more knowledgeable of the code wants to make a deeper change, feel free.

reames updated this revision to Diff 188065.Feb 23 2019, 4:46 PM

Rebase after landing SystemZ and Hexagon portions which had been previously LGTMed.

ping to ARM and Lanai maintainers

reames retitled this revision from Be super conservative about atomics in various backends to [ARM, Lanai] Be super conservative about atomics.Feb 23 2019, 4:47 PM
chill added a subscriber: chill.Feb 25 2019, 1:57 AM
chill added inline comments.
lib/Target/ARM/ARMLoadStoreOptimizer.cpp
1584 ↗(On Diff #188065)

How about using MachineMemOperand::isUnordered() instead?

john.brawn added inline comments.
lib/Target/ARM/ARMLoadStoreOptimizer.cpp
1584 ↗(On Diff #188065)

While it's probably valid to combine multiple unordered loads/stores into an ldm/stm (and also monotonic ones as well I think) the atomicity gets lost and the ldm/stm is not marked as atomic. I don't know if that actually leads to any problems, but I think we should be safe and just reject all atomic instructions for now. Maybe the TODO should be changed to something like

// TODO: We could allow unordered and monotonic atomics here, but we need to make sure the resulting ldm/stm is correctly marked as atomic.
jpienaar accepted this revision.Feb 25 2019, 7:13 AM
jpienaar added a subscriber: jpienaar.

Change to Lanai seems fine. The model for Lanai execution/usage means atomics aren't important, but treating them conservatively is better. Thanks!

This revision is now accepted and ready to land.Feb 25 2019, 7:13 AM
reames updated this revision to Diff 188209.Feb 25 2019, 9:48 AM
reames retitled this revision from [ARM, Lanai] Be super conservative about atomics to [ARM] Be super conservative about atomics.

Rebase after landing Lanai and addressing ARM comment. Pending LGTM from ARM contributors.

This revision was automatically updated to reflect the committed changes.