This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Support non-strictly stronger memory orderings in SIMemoryLegalizer
ClosedPublic

Authored by t-tye on Jul 23 2021, 5:09 PM.

Details

Summary

C++20 no longer requires the failure memory ordering to be no stronger than the
success memory ordering. Adjust assert in AMD GPU SIMemoryLegalizer, and merge
instruction memory orderings

Add common operation to merge memory orders that allows non strict memory
orderings to be combined. Use it in SIMemoryLegalizer and
MachineMemOperand::getMergedOrdering.

Diff Detail

Event Timeline

t-tye created this revision.Jul 23 2021, 5:09 PM
t-tye requested review of this revision.Jul 23 2021, 5:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2021, 5:09 PM
foad added inline comments.Jul 26 2021, 6:56 AM
llvm/include/llvm/Support/AtomicOrdering.h
140–141

You don't need any of these innermost parentheses.

foad added inline comments.Jul 26 2021, 6:58 AM
llvm/include/llvm/Support/AtomicOrdering.h
136

Typo "A0" for "AO".

No testcases?

llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp
656–657

Isn't this just equivalent to FailureOrdering = MMO->getFailureOrdering()?

efriedma added inline comments.Jul 26 2021, 10:34 AM
llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp
656–657

Oh, this is a loop; nevermind.

Has the IR atomic semantics been updated to match the C++20 change yet?

@arsenm See D98995, and my followups D103284/D103342. Not sure if everything is 100% fixed, but at least the basics are working.

t-tye updated this revision to Diff 361964.Jul 27 2021, 3:32 AM

Remove unnecessary ()s.

Correct comment A0->AO.

Added tests.

t-tye added a comment.Jul 27 2021, 3:34 AM

No testcases?

Added tests.

llvm/include/llvm/Support/AtomicOrdering.h
136

Corrected.

140–141

Removed innermost parenthesis.

Legalizer part LGTM. Not sure though how will it affect the rest of the languages since it implements C++20.

t-tye added a comment.Aug 7 2021, 3:46 PM

@rampitec I believe the other changes do not change the behavior from the perspective of the C++ 20 standard.

@efriedma do those changes look ok?

efriedma accepted this revision.Aug 9 2021, 12:15 PM

Haven't looked at the test changes in detail, but code changes look fine.

This revision is now accepted and ready to land.Aug 9 2021, 12:15 PM
t-tye added a comment.Aug 10 2021, 1:39 AM

Haven't looked at the test changes in detail, but code changes look fine.

Thanks @efriedma. @rampitec reviewed the other code so I will proceed to commit.

This revision was landed with ongoing or failed builds.Aug 10 2021, 1:44 AM
This revision was automatically updated to reflect the committed changes.