[PATCH] D52416: Allow FP types for atomicrmw xchg

[James Y Knight via Phabricator via llvm-commits]

jyknight accepted this revision.
jyknight added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:500
+
+  bool NeedBitcast = OrigTy->isFloatingPointTy();
+  if (NeedBitcast) {
----------------
arsenm wrote:
> asb wrote:
> > arsenm wrote:
> > > jfb wrote:
> > > > arsenm wrote:
> > > > > jfb wrote:
> > > > > > jyknight wrote:
> > > > > > > arsenm wrote:
> > > > > > > > jyknight wrote:
> > > > > > > > > Add a comment here that this code can go away if the cmpxchg instruction adds support for floating point types.
> > > > > > > > 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
> > > > > > > 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)
> > > > > > This is what various ISAs and programming languages do, so I support what James says :)
> > > > > AMDGPU does also have fcmpxchg, which does an FP compare, with nans failing etc.
> > > > Off topic... but wat?!? Once you have a NaN your fcmpxchg infloops? Count me confused.
> > > 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.
> > 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?
> To be clear I think fcmpxchg is it's own operation separate from the current cmpxchg inst
I hope we never support "fcmpxchg" -- that is, using a floating point semantic comparison. I'm having trouble imagining when that could ever be a useful operation.

Whether or not we support bitwise cmpxchg with FP types I'm pretty agnostic to. If there's some reason why it's useful to do so, we should. If there isn't, maybe we should or maybe we shouldn't.

But -- to the point of this thread: there's now a comment here that this code should be removed if we do so, which is all I really wanted. :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52416/new/

https://reviews.llvm.org/D52416