Page MenuHomePhabricator

Allow FP types for atomicrmw xchg
Needs ReviewPublic

Authored by arsenm on Sep 24 2018, 6:39 AM.

Diff Detail

Event Timeline

arsenm created this revision.Sep 24 2018, 6:39 AM
jfb added a comment.Sep 24 2018, 9:35 AM

What does this look like with half, fp80, fp128?
Does this not change anything on other architectures?

lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
2172

I don't get why only FP16 is accessible here.

lib/Target/AMDGPU/AMDGPUISelLowering.cpp
290 ↗(On Diff #166663)

?

arsenm marked an inline comment as done.Oct 1 2018, 9:25 PM
arsenm added inline comments.
lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
2172

We don't support any other cases where a narrower FP type is illegal, but a wider one is. That would require a generalized version of FP16_TO_FP/FP_TO_FP16. There aren't any in tree targets that support f64 but not f32 for example.

I've removed this assertion, so this will fail in GetPromotionOpcode for any other type like any other legalization would

arsenm updated this revision to Diff 167881.Oct 1 2018, 9:26 PM

Fix handling with wider illegal FP types. Also avoid directly using the f16 legalization instructions

arsenm added a comment.Oct 1 2018, 9:30 PM
In D52416#1243625, @jfb wrote:

What does this look like with half, fp80, fp128?
Does this not change anything on other architectures?

f80 is illegal since it's not a power of two sized type, so that's rejected by the IR verifier. I guess supporting it could somehow be possible, but is beyond the scope of this patch. I added some tests for fp128.

Ideally each target would add support for the FP types directly in its selection patterns, but that's a lot of work. The individual targets avoid work by setting the defaults to the bitcast lowering. For the atomic_load/store FP support, this for some reason was done in the IR AtomicExpand pass.

arsenm updated this revision to Diff 171990.Oct 31 2018, 12:10 PM

Fix bad patch split

arsenm updated this revision to Diff 173919.Nov 13 2018, 12:52 PM

Fix test split issue

Should there be a change to LangRef?

Apart from that this looks good to me.

asb added reviewers: asb, jyknight.EditedThu, Nov 29, 7:27 AM
asb added a subscriber: asb.

I should probably plan to reflect support for this in my in-flight cmpxchg patch (which adds target-independent support for late lowering of cmpxchg in the same way I added it for atomicrmw). [incidentally - reviews on that patch would be very welcome...]

Adding James Y Knight as a reviewer here too.

EDIT: worth actually linking to my cmpxchg patch! D48131

I had the LangRef change but it must have gotten dropped somehow

arsenm updated this revision to Diff 175922.Thu, Nov 29, 12:03 PM

Restore langref change

Looks reasonable.

Perhaps add support for pointer types too? Supporting the same set of types for load, store, xchg, and cmpxchg (eventually) would seem sensible.

lib/CodeGen/AtomicExpandPass.cpp
501

Add a comment here that this code can go away if the cmpxchg instruction adds support for floating point types.

arsenm marked an inline comment as done.Wed, Dec 5, 12:51 PM
arsenm added inline comments.
lib/CodeGen/AtomicExpandPass.cpp
501

I think there might need to be a separate fcmpxchg instruction for that, unless you mean there will also be a version that treats the FP type here as integer in memory

jyknight added inline comments.Thu, Dec 6, 8:40 AM
lib/CodeGen/AtomicExpandPass.cpp
501

No -- the intent is not to compare for floating-point-equality ala fcmp, but rather just as bit equality. (e.g. NaNs are equal to each-other when they have the same bit representation, and unequal if they do not)

jfb added inline comments.Thu, Dec 6, 9:03 AM
lib/CodeGen/AtomicExpandPass.cpp
501

This is what various ISAs and programming languages do, so I support what James says :)

arsenm marked an inline comment as done.Thu, Dec 6, 9:04 AM
arsenm added inline comments.
lib/CodeGen/AtomicExpandPass.cpp
501

AMDGPU does also have fcmpxchg, which does an FP compare, with nans failing etc.

jfb added inline comments.Thu, Dec 6, 9:09 AM
lib/CodeGen/AtomicExpandPass.cpp
501

Off topic... but wat?!? Once you have a NaN your fcmpxchg infloops? Count me confused.

arsenm marked an inline comment as done.Thu, Dec 6, 9:44 AM

xxz

lib/CodeGen/AtomicExpandPass.cpp
501

It doesn't let you put the NaN in? As long as the memory was initialized with something not-NaN before I think it works? Otherwise you're stuck with NaNs forever. I haven't tried using it, but that's my interpretation of the manual.

asb added inline comments.Thu, Dec 6, 9:45 AM
lib/CodeGen/AtomicExpandPass.cpp
501

atomicrmw xchg with FP types makes sense - the semantics are unambiguous. Is it really worth the potential confusion of what fp cmpxchg means vs just sticking with bitcast + integer cmpxchg?

arsenm marked an inline comment as done.Thu, Dec 6, 9:48 AM
arsenm added inline comments.
lib/CodeGen/AtomicExpandPass.cpp
501

To be clear I think fcmpxchg is it's own operation separate from the current cmpxchg inst

arsenm added a comment.Thu, Dec 6, 9:54 AM

Looks reasonable.

Perhaps add support for pointer types too? Supporting the same set of types for load, store, xchg, and cmpxchg (eventually) would seem sensible.

Yes, I think that's a separate step though

arsenm updated this revision to Diff 177001.Thu, Dec 6, 9:55 AM

Add requested comment