This is an archive of the discontinued LLVM Phabricator instance.

Differential D106729

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

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

Details

Reviewers
kzhuravl
scott.linder
rampitec
efriedma
arsenm
Commits
rG53eb469195b8: [AMDGPU] Support non-strictly stronger memory orderings in SIMemoryLegalizer
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

Unit TestsFailed

TimeTest
2,740 msx64 debian > libarcher.critical::critical.c
2,850 msx64 debian > libarcher.critical::lock-nested.c
3,000 msx64 debian > libarcher.races::critical-unrelated.c
2,530 msx64 debian > libarcher.races::lock-nested-unrelated.c
3,090 msx64 debian > libarcher.races::lock-unrelated.c
View Full Test Results (16 Failed)

Event Timeline

t-tye created this revision.Jul 23 2021, 5:09 PM
Herald added subscribers: foad, dexonsmith, kerbowa and 8 others. · View Herald TranscriptJul 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
Herald added subscribers: llvm-commits, wdng. · View Herald Transcript
t-tye added a reviewer: efriedma.Jul 23 2021, 5:49 PM
Harbormaster completed remote builds in B115979: Diff 361386.Jul 23 2021, 6:18 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".

JonChesterfield added a subscriber: JonChesterfield.Jul 26 2021, 8:37 AM
t-tye added a reviewer: arsenm.Jul 26 2021, 8:37 AM
efriedma added a comment.Jul 26 2021, 10:32 AM

No testcases?

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

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

Oh, this is a loop; nevermind.

arsenm added a comment.Jul 26 2021, 10:36 AM

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

efriedma added a comment.Jul 26 2021, 10:42 AM

@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
In D106729#2904861, @efriedma wrote:

No testcases?

Added tests.

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

Corrected.

140–141

Removed innermost parenthesis.

Harbormaster completed remote builds in B116385: Diff 361964.Jul 27 2021, 5:01 AM
rampitec added a comment.Aug 6 2021, 11:10 AM

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
In D106729#2935273, @efriedma wrote:

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
Closed by commit rG53eb469195b8: [AMDGPU] Support non-strictly stronger memory orderings in SIMemoryLegalizer (authored by t-tye). · Explain Why
This revision was automatically updated to reflect the committed changes.
t-tye added a commit: rG53eb469195b8: [AMDGPU] Support non-strictly stronger memory orderings in SIMemoryLegalizer.

Revision Contents

PathSize
llvm/
include/
llvm/
CodeGen/
12 lines
Support/
10 lines
lib/
Target/
AMDGPU/
10 lines

Diff 361386

llvm/include/llvm/CodeGen/MachineMemOperand.h

Show First 20 Lines • Show All 276 Lines • ▼ Show 20 Linespublic:
AtomicOrdering getFailureOrdering() const { AtomicOrdering getFailureOrdering() const {
return static_cast<AtomicOrdering>(AtomicInfo.FailureOrdering); return static_cast<AtomicOrdering>(AtomicInfo.FailureOrdering);
} }
/// Return a single atomic ordering that is at least as strong as both the /// Return a single atomic ordering that is at least as strong as both the
/// success and failure orderings for an atomic operation. (For operations /// success and failure orderings for an atomic operation. (For operations
/// other than cmpxchg, this is equivalent to getSuccessOrdering().) /// other than cmpxchg, this is equivalent to getSuccessOrdering().)
AtomicOrdering getMergedOrdering() const { AtomicOrdering getMergedOrdering() const {
AtomicOrdering Ordering = getSuccessOrdering(); return getMergedAtomicOrdering(getSuccessOrdering(), getFailureOrdering());
AtomicOrdering FailureOrdering = getFailureOrdering();
if (FailureOrdering == AtomicOrdering::SequentiallyConsistent)
return AtomicOrdering::SequentiallyConsistent;
if (FailureOrdering == AtomicOrdering::Acquire) {
if (Ordering == AtomicOrdering::Monotonic)
return AtomicOrdering::Acquire;
if (Ordering == AtomicOrdering::Release)
return AtomicOrdering::AcquireRelease;
}
return Ordering;
} }
bool isLoad() const { return FlagVals & MOLoad; } bool isLoad() const { return FlagVals & MOLoad; }
bool isStore() const { return FlagVals & MOStore; } bool isStore() const { return FlagVals & MOStore; }
bool isVolatile() const { return FlagVals & MOVolatile; } bool isVolatile() const { return FlagVals & MOVolatile; }
bool isNonTemporal() const { return FlagVals & MONonTemporal; } bool isNonTemporal() const { return FlagVals & MONonTemporal; }
bool isDereferenceable() const { return FlagVals & MODereferenceable; } bool isDereferenceable() const { return FlagVals & MODereferenceable; }
bool isInvariant() const { return FlagVals & MOInvariant; } bool isInvariant() const { return FlagVals & MOInvariant; }
▲ Show 20 Lines • Show All 66 Lines • Show Last 20 Lines

llvm/include/llvm/Support/AtomicOrdering.h

Show First 20 Lines • Show All 127 Lines • ▼ Show 20 Lines
inline bool isAcquireOrStronger(AtomicOrdering AO) { inline bool isAcquireOrStronger(AtomicOrdering AO) {
return isAtLeastOrStrongerThan(AO, AtomicOrdering::Acquire); return isAtLeastOrStrongerThan(AO, AtomicOrdering::Acquire);
} }
inline bool isReleaseOrStronger(AtomicOrdering AO) { inline bool isReleaseOrStronger(AtomicOrdering AO) {
return isAtLeastOrStrongerThan(AO, AtomicOrdering::Release); return isAtLeastOrStrongerThan(AO, AtomicOrdering::Release);
} }
/// Return a single atomic ordering that is at least as strong as both the \p A0
foadUnsubmitted
Not Done
ReplyInline Actions

Typo "A0" for "AO".

foad: Typo "A0" for "AO".
t-tyeAuthorUnsubmitted
Done
ReplyInline Actions

Corrected.

t-tye: Corrected.
/// and \p Other orderings for an atomic operation.
inline AtomicOrdering getMergedAtomicOrdering(AtomicOrdering AO,
AtomicOrdering Other) {
if (((AO == AtomicOrdering::Acquire) && (Other == AtomicOrdering::Release)) ||
((AO == AtomicOrdering::Release) && (Other == AtomicOrdering::Acquire)))
foadUnsubmitted
Not Done
ReplyInline Actions

You don't need any of these innermost parentheses.

foad: You don't need any of these innermost parentheses.
t-tyeAuthorUnsubmitted
Done
ReplyInline Actions

Removed innermost parenthesis.

t-tye: Removed innermost parenthesis.
return AtomicOrdering::AcquireRelease;
return isStrongerThan(AO, Other) ? AO : Other;
}
inline AtomicOrderingCABI toCABI(AtomicOrdering AO) { inline AtomicOrderingCABI toCABI(AtomicOrdering AO) {
static const AtomicOrderingCABI lookup[8] = { static const AtomicOrderingCABI lookup[8] = {
/* NotAtomic */ AtomicOrderingCABI::relaxed, /* NotAtomic */ AtomicOrderingCABI::relaxed,
/* Unordered */ AtomicOrderingCABI::relaxed, /* Unordered */ AtomicOrderingCABI::relaxed,
/* relaxed */ AtomicOrderingCABI::relaxed, /* relaxed */ AtomicOrderingCABI::relaxed,
/* consume */ AtomicOrderingCABI::consume, /* consume */ AtomicOrderingCABI::consume,
/* acquire */ AtomicOrderingCABI::acquire, /* acquire */ AtomicOrderingCABI::acquire,
/* release */ AtomicOrderingCABI::release, /* release */ AtomicOrderingCABI::release,
Show All 9 Lines

llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp

Show First 20 Lines • Show All 120 Lines • ▼ Show 20 Lines if (Ordering == AtomicOrdering::NotAtomic) {
FailureOrdering == AtomicOrdering::NotAtomic); FailureOrdering == AtomicOrdering::NotAtomic);
return; return;
} }
assert(Scope != SIAtomicScope::NONE && assert(Scope != SIAtomicScope::NONE &&
(OrderingAddrSpace & SIAtomicAddrSpace::ATOMIC) != (OrderingAddrSpace & SIAtomicAddrSpace::ATOMIC) !=
SIAtomicAddrSpace::NONE && SIAtomicAddrSpace::NONE &&
(InstrAddrSpace & SIAtomicAddrSpace::ATOMIC) != (InstrAddrSpace & SIAtomicAddrSpace::ATOMIC) !=
SIAtomicAddrSpace::NONE && SIAtomicAddrSpace::NONE);
!isStrongerThan(FailureOrdering, Ordering));
// There is also no cross address space ordering if the ordering // There is also no cross address space ordering if the ordering
// address space is the same as the instruction address space and // address space is the same as the instruction address space and
// only contains a single address space. // only contains a single address space.
if ((OrderingAddrSpace == InstrAddrSpace) && if ((OrderingAddrSpace == InstrAddrSpace) &&
isPowerOf2_32(uint32_t(InstrAddrSpace))) isPowerOf2_32(uint32_t(InstrAddrSpace)))
this->IsCrossAddressSpaceOrdering = false; this->IsCrossAddressSpaceOrdering = false;
▲ Show 20 Lines • Show All 507 Lines • ▼ Show 20 Lines if (OpOrdering != AtomicOrdering::NotAtomic) {
MMI->isSyncScopeInclusion(SSID, MMO->getSyncScopeID()); MMI->isSyncScopeInclusion(SSID, MMO->getSyncScopeID());
if (!IsSyncScopeInclusion) { if (!IsSyncScopeInclusion) {
reportUnsupported(MI, reportUnsupported(MI,
"Unsupported non-inclusive atomic synchronization scope"); "Unsupported non-inclusive atomic synchronization scope");
return None; return None;
} }
SSID = IsSyncScopeInclusion.getValue() ? SSID : MMO->getSyncScopeID(); SSID = IsSyncScopeInclusion.getValue() ? SSID : MMO->getSyncScopeID();
Ordering = isStrongerThan(Ordering, OpOrdering) Ordering = getMergedAtomicOrdering(Ordering, OpOrdering);
? Ordering
: MMO->getSuccessOrdering();
assert(MMO->getFailureOrdering() != AtomicOrdering::Release && assert(MMO->getFailureOrdering() != AtomicOrdering::Release &&
MMO->getFailureOrdering() != AtomicOrdering::AcquireRelease); MMO->getFailureOrdering() != AtomicOrdering::AcquireRelease);
FailureOrdering = FailureOrdering =
efriedmaUnsubmitted
Not Done
ReplyInline Actions

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

efriedma: Isn't this just equivalent to `FailureOrdering = MMO->getFailureOrdering()`?
efriedmaUnsubmitted
Not Done
ReplyInline Actions

Oh, this is a loop; nevermind.

efriedma: Oh, this is a loop; nevermind.
isStrongerThan(FailureOrdering, MMO->getFailureOrdering()) ? getMergedAtomicOrdering(FailureOrdering, MMO->getFailureOrdering());
FailureOrdering : MMO->getFailureOrdering();
} }
} }
SIAtomicScope Scope = SIAtomicScope::NONE; SIAtomicScope Scope = SIAtomicScope::NONE;
SIAtomicAddrSpace OrderingAddrSpace = SIAtomicAddrSpace::NONE; SIAtomicAddrSpace OrderingAddrSpace = SIAtomicAddrSpace::NONE;
bool IsCrossAddressSpaceOrdering = false; bool IsCrossAddressSpaceOrdering = false;
if (Ordering != AtomicOrdering::NotAtomic) { if (Ordering != AtomicOrdering::NotAtomic) {
auto ScopeOrNone = toSIAtomicScope(SSID, InstrAddrSpace); auto ScopeOrNone = toSIAtomicScope(SSID, InstrAddrSpace);
▲ Show 20 Lines • Show All 1,198 Lines • Show Last 20 Lines