[PATCH] D53965: IR: Add fp operations to atomicrmw

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

I'd note that this change isn't a necessary prerequisite to implementing the C++ feature -- FP math is already available in C11, and clang already implements it by lowering to cmpxchg. I'd be very skeptical of this addition, if you hadn't mentioned that AMDGPU supported it in hardware. AFAIK, there's no other ISAs that support atomic fadd in hardware. However, that AMDGPU does have hardware support seems like a sufficient reason to add support for it to LLVM.

On LL/SC architectures, while you could theoretically execute a floating-point instruction inside the LL/SC loop, there's often a bunch of complexity regarding the potential for a trap to be raised (e.g. for software FPU emulation, or for edge cases like denormals), which may deterministically cancel the reservation, and create an infinite loop. So I'm pretty wary of any proposal to lower to an LL/SC loop containing FP instructions -- I think even on LL/SC architectures, this should generally be lowered to a cmpxchg loop.

On a general note -- this patch doesn't seem complete. I think this should be a complete implementation of the feature -- specifying an "atomicrmw fadd" should function on all architectures after this, even if not most optimally on AMDGPU (since it's not possible to lower into SelectionDAG until after the next patch).

That'll means pulling in the AtomicExpand changes, and also updating all architectures' shouldExpandAtomicRMWInIR to return CmpXchg for these ops.



================
Comment at: docs/LangRef.rst:8443
+the ``atomicrmw`` is marked as ``volatile``, then the optimizer is not
+allowed to modify the number or order of execution of
 
----------------
Lost some words.


================
Comment at: include/llvm/IR/Instructions.h:813
 
+  bool isFloatingPointOperation() const {
+    return isFPOperation(getOperation());
----------------
Unused?


================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:1522-1523
     return {};
+  default:
+    llvm_unreachable("unhandled operation");
   }
----------------
This isn't great. And I see in your next patch you fix it, but splitting that to another patch doesn't make sense.


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

https://reviews.llvm.org/D53965