This is an archive of the discontinued LLVM Phabricator instance.

[X86] Make wide loads be managed by AtomicExpand
ClosedPublic

Authored by morisset on Sep 18 2014, 3:52 PM.

Details

Summary

AtomicExpand already had logic for expanding wide loads and stores on LL/SC
architectures, and for expanding wide stores on CmpXchg architectures, but
not for wide loads on CmpXchg architectures. This patch fills this hole,
and makes use of this new feature in the X86 backend.

Only one functionnal change: we now lose the SynchScope attribute.
It is regrettable, but I have another patch that I will submit soon that will
solve this for all of AtomicExpand (it seemed better to split it apart as it
is a different concern).

Diff Detail

Event Timeline

morisset updated this revision to Diff 13854.Sep 18 2014, 3:52 PM
morisset retitled this revision from to [X86] Make wide loads be managed by AtomicExpand.
morisset updated this object.
morisset edited the test plan for this revision. (Show Details)
morisset added a reviewer: jfb.
morisset added a subscriber: Unknown Object (MLST).
jfb edited edge metadata.Sep 22 2014, 1:10 PM

The tests all stay the same after this change? Were they actually sufficient? In particular, I want to make sure that the lock prefix remains where it should be.

lib/Target/X86/X86ISelLowering.cpp
17572

These FIXMEs should probably be kept. How would they get fixed with this new infrastructure? shouldExpandAtomicLoadInIR would change to take that into account, and ReplaceATOMIC_LOAD would do the work?

The tests for cmpxchg16b were indeed testing for the lock prefix, but not the tests for the 8b version. I will upload a new patch with an improved test (and preserved FIXME).

lib/Target/X86/X86ISelLowering.cpp
17572

The second comment looks obsolete to me: 16-bytes Cmpxchg definitely works on x86 in recent versions of LLVM, and there are lots of tests of it.
The first comment may indeed more interesting to preserve, I will keep it in this file (in shouldExpandAtomicLoad). If someone wants to try it someday, they will have to make shouldExpandAtomicLoad return false for this case, and do the lowering themselves (as it is a completely target-dependent trick).

morisset updated this revision to Diff 14000.Sep 23 2014, 10:01 AM
morisset edited edge metadata.

Preserve FIXME comment, and refine tests.

jfb added inline comments.Sep 23 2014, 10:17 AM
test/CodeGen/X86/atomic-load-store-wide.ll
1

Is there also a cmpxchg8b test for x86-64?

9

Use CHECK-LABEL and CHECK-NEXT.

morisset added inline comments.Sep 23 2014, 10:24 AM
test/CodeGen/X86/atomic-load-store-wide.ll
1

I do not understand this point, 64 bits accesses are naturally atomic on x86-64 (provided some alignment constraints are respected).
So there is no need for cmpxchg8b, and I do not know a case where they can be emitted by LLVM on x86-64.

9

Thanks, fixed.

morisset updated this revision to Diff 14001.Sep 23 2014, 10:25 AM

Use CHECK-LABEL and CHECK-NEXT

jfb accepted this revision.Sep 23 2014, 10:34 AM
jfb edited edge metadata.
jfb added inline comments.
test/CodeGen/X86/atomic-load-store-wide.ll
1

Oh right! Ignore :)

This revision is now accepted and ready to land.Sep 23 2014, 10:34 AM
morisset closed this revision.Sep 23 2014, 2:09 PM
morisset updated this revision to Diff 14020.

Closed by commit rL218332 (authored by @morisset).