This is an archive of the discontinued LLVM Phabricator instance.

[X86] Use FILD/FIST to implement i64 atomic load on 32-bit targets with X87, but no SSE2
ClosedPublic

Authored by craig.topper on Apr 2 2019, 3:04 PM.

Details

Summary

If we have X87, but not SSE2 we can atomicaly load an i64 value into the significand of an 80-bit extended precision x87 register using fild. We can then use a fist instruction to convert it back to an i64 integer and store it to a stack temporary. From there we can do two 32-bit loads to get the value into integer registers without worrying about atomicness.

This matches what gcc and icc do for this case and removes an existing FIXME.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Apr 2 2019, 3:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2019, 3:04 PM
craig.topper marked an inline comment as done.Apr 2 2019, 3:05 PM
craig.topper added inline comments.
llvm/test/CodeGen/X86/misched_phys_reg_assign_order.ll
52 ↗(On Diff #193370)

this test seemed to be testing somethign about the cmpxchg sequence so I've added noimplicitfloat to prevent the new code from triggering.

reames added a comment.Apr 3 2019, 9:17 AM

Minor comments only. I'll leave approval to someone more familiar w/x87 if possible

llvm/lib/Target/X86/X86ISelLowering.cpp
27434 ↗(On Diff #193370)

You should add a test for a volatile atomic load. Your code is correct for this case, but it doesn't appear tested.

27440 ↗(On Diff #193370)

The fact that this store *isn't* atomic or volatile deserves a comment.

llvm/test/CodeGen/X86/atomic-non-integer.ll
232 ↗(On Diff #193370)

If I'm understanding this sequence correctly - not sure I am - it looks like we could simply load directly from your inserted stack temporary instead of shuffling into another and then loading. Might be worth a TODO

Add volatile atomic test case.
Add comment about the stack tempoary access not being atomic
Add FIXME for the case where we are already doing a store.

RKSimon added inline comments.Apr 9 2019, 1:13 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
27431 ↗(On Diff #193575)

Do we?

craig.topper marked an inline comment as done.Apr 9 2019, 12:12 PM
craig.topper added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
27431 ↗(On Diff #193575)

I based that on the similar sequence in X86TargetLowering::BuildFILD. Wasn't sure if the FIXME there was still relevant.

Rebase due to test file splitting.

Add coment on the glue FIXME pointing to the FIXME in BuildFILD

RKSimon accepted this revision.Apr 11 2019, 3:29 AM

LGTM

llvm/test/CodeGen/X86/atomic-load-store-wide.ll
121 ↗(On Diff #194386)

pre-commit this?

This revision is now accepted and ready to land.Apr 11 2019, 3:29 AM