Page MenuHomePhabricator

Mutate TargetLowering::shouldExpandAtomicRMWInIR to specifically dictate how AtomicRMWInsts are expanded.
ClosedPublic

Authored by DiamondLovesYou on Feb 17 2015, 7:37 PM.

Details

Summary

In PNaCl, most atomic instructions have their own @llvm.nacl.atomic.* function, each one, with a few exceptions, represents a consistent behaviour across all NaCl-supported targets. Unfortunately, the atomic RMW operations nand, [u]min, and [u]max aren't directly represented by any such @llvm.nacl.atomic.* function. This patch refines shouldExpandAtomicRMWInIR in TargetLowering so that a future Le32TargetLowering class can selectively inform the caller how the target desires the atomic RMW instruction to be expanded (ie via load-linked/store-conditional for ARM/AArch64, via cmpxchg for X86/others?, or not at all for Mips) if at all.

This does not represent a behavioural change and as such no tests were added.

Diff Detail

Repository
rL LLVM

Event Timeline

DiamondLovesYou retitled this revision from to Mutate TargetLowering::shouldExpandAtomicRMWInIR to specifically dictate how AtomicRMWInsts are expanded..
DiamondLovesYou updated this object.
DiamondLovesYou edited the test plan for this revision. (Show Details)
DiamondLovesYou added a reviewer: jfb.
DiamondLovesYou set the repository for this revision to rL LLVM.
DiamondLovesYou added a subscriber: Unknown Object (MLST).

No tests? No explanation?

include/llvm/Target/TargetLowering.h
1083 ↗(On Diff #20135)

Does any compiler really whinge about this?

No tests?

It seems to me that the test would be passing existing tests, considering this patch happens only at an API boundary and doesn't change the behaviour of existing code.

No explanation?

Apologies; I've added a summary.

jfb edited edge metadata.Feb 18 2015, 9:32 AM

No tests?

It seems to me that the test would be passing existing tests, considering this patch happens only at an API boundary and doesn't change the behaviour of existing code.

You change description should explain that this change doesn't lead to a functional change, so the existing tests are all still valid.

include/llvm/Target/TargetLowering.h
128 ↗(On Diff #20135)

I would just call this None instead of Native.

lib/CodeGen/AtomicExpandPass.cpp
143 ↗(On Diff #20135)

I find the logic with continue confusing, could you instead do:

if (...) {
  MadeChange = true;
} else {
  MadeChange |= ...;
}
229 ↗(On Diff #20135)

I'd rename to tryExpandAtomicRMW since it doesn't expand them unconditionally anymore.

231 ↗(On Diff #20135)

clang-format.

I'd also just do:

switch (TLI->shouldExpandAtomicRMWInIR(AI)) {
237 ↗(On Diff #20135)

Use string concatenation instead of backslashes:

"foo "
"bar"

Is equivalent to:

"foo bar"
243 ↗(On Diff #20135)

Could you explain the rationale for splitting LLSC from CmpXhg instead of using the same logic as before? It does provide more flexibility, but the enum doesn't really explain why that flexibility is desirable, and doesn't point out that LLSC implies hasLoadLinkedStoreConditional: even if your assert checks this, it's a tricky gotcha for a Target implementer.

lib/Target/AArch64/AArch64ISelLowering.cpp
8782 ↗(On Diff #20135)

I think it's generally more idiomatic to do:

return Size <= 128 ? LLSC : Native;
lib/Target/ARM/ARMISelLowering.cpp
11110 ↗(On Diff #20135)

Same on ternary operator.

lib/Target/X86/X86ISelLowering.cpp
19622 ↗(On Diff #20135)

Same.

19641 ↗(On Diff #20135)

Same.

DiamondLovesYou edited edge metadata.

Addressed JF's comments.

jfb added a comment.Feb 18 2015, 10:49 AM

Looks good overall, but I'll let @t.p.northover comment more.

lib/CodeGen/AtomicExpandPass.cpp
218 ↗(On Diff #20202)

Why remove this?

Fix a comment from the last patchset.

jfb accepted this revision.Feb 24 2015, 9:15 AM
jfb edited edge metadata.

This lgtm, I'll leave open for a short while if anyone else wants to comment.

This revision is now accepted and ready to land.Feb 24 2015, 9:15 AM
jfb added a comment.Mar 2 2015, 11:27 AM

@DiamondLovesYou: could you re-upload this patch? It looks like the latest one is an incremental patch comparing your previous one, not a patch from tip-of-tree. Once that's done I'll be able to commit.

DiamondLovesYou edited edge metadata.

Fix use of an incremental diff.

This revision was automatically updated to reflect the committed changes.
jfb added a comment.Mar 4 2015, 7:51 AM

I also re-ran clang-format, some of the code wasn't formatted with it.

lib/CodeGen/AtomicExpandPass.cpp
244 ↗(On Diff #21136)

I removed the semicolon here, and had to add llvm_unreachable because not all compilers realize that all cases are handled.