This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][RISCV] Make sure isel correctly honors failure orderings.
ClosedPublic

Authored by efriedma on May 27 2021, 1:52 PM.

Details

Summary

If a cmpxchg specifies acquire or seq_cst on failure, make sure we generate code consistent with that ordering even if the success ordering is not acquire/seq_cst.

At one point, it was ambiguous whether this sort of construct was valid, but the C++ standad and LLVM now accept arbitrary combinations of success/failure orderings.

This doesn't address the corresponding issue in AtomicExpand. (This was reported as https://bugs.llvm.org/show_bug.cgi?id=33332 .)

Fixes https://bugs.llvm.org/show_bug.cgi?id=50512.

Diff Detail

Event Timeline

efriedma created this revision.May 27 2021, 1:52 PM
efriedma requested review of this revision.May 27 2021, 1:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2021, 1:52 PM
Herald added a subscriber: MaskRay. · View Herald Transcript
This revision is now accepted and ready to land.May 28 2021, 3:05 AM
jyknight added inline comments.May 28 2021, 5:51 AM
llvm/include/llvm/CodeGen/MachineMemOperand.h
261

I think this is a confusing API now. The reason it made sense to have getOrdering return the success ordering for a cmpxchg before is because that was always stronger than the failure ordering. So, using the success order was a safe "default" ordering. That's no longer the case.

Maybe we should have getOrdering should return the merged ordering, and rename the existing getOrdering to getSuccessOrdering.

Or, actually I like this better: remove getOrdering entirely, by renaming to getSuccessOrdering. Then we'll have getSuccessOrdering, getFailureOrdering, and getMergedOrdering. That's much clearer. (And, yes, this does still make sense for non-cmpxchg -- they always succeed, so the success ordering is the only relevant thing for them.)

efriedma added inline comments.May 28 2021, 12:15 PM
llvm/include/llvm/CodeGen/MachineMemOperand.h
261

So the idea is basically just rename getOrdering()->getSuccessOrdering()? Sure, I'll look at doing that in a followup.

This revision was landed with ongoing or failed builds.May 28 2021, 12:48 PM
This revision was automatically updated to reflect the committed changes.