Page MenuHomePhabricator

[X86] Use the generic AtomicExpandPass instead of X86AtomicExpandPass
ClosedPublic

Authored by morisset on Aug 27 2014, 2:36 PM.

Details

Reviewers
jfb
Summary

This required a new hook called hasLoadLinkedStoreConditional to know whether
to expand atomics to LL/SC (ARM, AArch64, in a future patch Power) or to
CmpXchg (X86).

Apart from that, the new code in AtomicExpandPass is mostly moved from
X86AtomicExpandPass. The main result of this patch is to get rid of that
pass, which had lots of code duplicated with AtomicExpandPass.

Depends on D4960 and D5035.

Diff Detail

Event Timeline

morisset updated this revision to Diff 13002.Aug 27 2014, 2:36 PM
morisset retitled this revision from to [X86] Use the generic AtomicExpandPass instead of X86AtomicExpandPass.
morisset updated this object.
morisset edited the test plan for this revision. (Show Details)
morisset added a reviewer: jfb.
morisset added subscribers: t.p.northover, reames, Unknown Object (MLST).
morisset updated this revision to Diff 13008.Aug 27 2014, 3:57 PM

Fixed formatting with clang-format.

jfb edited edge metadata.Aug 30 2014, 11:21 AM

Were there previously tests for the functionality you're moving? If not, or if they were insufficient, could you augment the tests?

I've never been quite comfortable with synchronization scope, but I just realized that your recent work does away with it. Unconditionally upgrading to cross-thread is OK, but do you think it would be worth keeping the synchronization scope property when it's not cross-thread? I'm not sure how it's actually used (I understand the implications for signal handling, but I'm not sure other parts of the code base are correct). In any case, this should be a separate change.

lib/CodeGen/AtomicExpandPass.cpp
135

IIRC they both need a lock prefix to be atomic.

261

I'd rename this to FenceOrder.

lib/Target/X86/X86ISelLowering.cpp
16784

Fix the FIXME?

jfb added inline comments.Aug 30 2014, 1:20 PM
lib/CodeGen/AtomicExpandPass.cpp
298

Could you also explain the rationale behind the alignment (C11/C++11 memory model requires at least natural alignment, and the IR has the same guarantee).

There was already a lot of tests for atomics on X86 (test/CodeGen/X86/atomic*), so I did not modify them in this patch, just checked that they still pass.

I agree that preserving the synchronization scope might be good, but that would be a different patch (the code in X86AtomicAtomicPass does not look like it was preserving synchronization scope either).

lib/CodeGen/AtomicExpandPass.cpp
135

Indeed, I will update the comment.

298

All of this code was just moved from the X86AtomicExpandPass, I am not entirely sure of why it does things in a specific way.
But is it not providing exactly this natural alignment that you mention ?

lib/Target/X86/X86ISelLowering.cpp
16784

This code was moved from X86AtomicExpandPass. Because fixing this FIXME would not be entirely trivial (need to add hasCmpxchg8b to the subtarget, find which ones offer it, and so on), and is orthogonal to this change, I chose not to tackle it in this patch.

morisset updated this revision to Diff 13222.Sep 3 2014, 11:53 AM
morisset edited edge metadata.

Update comment.

morisset updated this revision to Diff 13223.Sep 3 2014, 11:54 AM

Fix wrong update

jfb added a comment.Sep 3 2014, 1:46 PM
In D5090#10, @morisset wrote:

There was already a lot of tests for atomics on X86 (test/CodeGen/X86/atomic*), so I did not modify them in this patch, just checked that they still pass.

Sounds good.

I agree that preserving the synchronization scope might be good, but that would be a different patch (the code in X86AtomicAtomicPass does not look like it was preserving synchronization scope either).

Sounds good.

lib/CodeGen/AtomicExpandPass.cpp
298

Yes, but to an unfamiliar reader it's not obvious why this alignment is the right one, so I think a comment that says "same as C++11 guarantees" would be helpful.

lib/Target/X86/X86ISelLowering.cpp
16774

"1 step up" is confusing, I'd just say "larger than the native atomic store width".

16784

Sounds good.

morisset updated this revision to Diff 13332.Sep 5 2014, 11:24 AM

Improve comments.

ping. Is this patch good to go ? There are several other patches with dependencies on it (such as D5179 - also needing review, and D5180 - that already received an LGTM).

Thanks,
Robin

jfb accepted this revision.Sep 16 2014, 3:29 PM
jfb edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Sep 16 2014, 3:29 PM

Thanks !
I will commit it soon (currently testing after rebase on trunk).

morisset closed this revision.Sep 16 2014, 5:22 PM

r217928