[PATCH] D15471: [IR] Add support for floating pointer atomic loads and stores

[Philip Reames via llvm-commits]

reames added a comment.

In http://reviews.llvm.org/D15471#311041, @jfb wrote:

> For x86, can you add a negative test for `fp80` to make sure it doesn't work? Or will that test only hit an assert? I'm assuming that it should fail validation.


This will be caught by the verifier.  Will add a test.

> Also, add `fp128`. For now I'm assuming we'll do `lock cmpxchg16b` on processors which support it, and a call to the runtime lock from compiler-rt otherwise (`__atomic_load_16` and `__atomic_store_16` from `lib/builtins/atomic.c` which I'm not sure work?).


I haven't checked, but the details of the lowering (including correctness!) are beyond the scope of this change.  The lowering will be whatever you would have gotten with the previous bitcast idiom.  *All* this change is doing is removing the need for that idiom.  If we need to revise lowering, that should and will be a separate change.  I will add a test to check that we lower to *something*, but that's all that should be part of this change.

I'll make the testing changes requested.  With the assumption those will be done before submission, can I get an official LGTM?

I'm noticing a trend for this patch to keep snowballing to include more and more lowering questions; I specifically want to cut that off.  This change *shouldn't* be changing lowering in any way from what frontends get today.  That's the entire point of the patch.


================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:284
@@ -224,1 +283,3 @@
 
+/// Convert an atomic store of a non-integeral type to an integer store of the
+/// equivelent bitwidth.  We used to not support floating point or vector
----------------
jfb wrote:
> "integral"
Will fix.

================
Comment at: test/CodeGen/X86/atomic-non-integer.ll:1
@@ +1,2 @@
+; RUN: llc < %s -mtriple=x86_64-linux-generic -verify-machineinstrs -mattr=sse2 | FileCheck %s
+
----------------
jfb wrote:
> Add a reference to:
> Also add a reference to: https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html
This is, or should, be documented in the code.  Since I didn't write the lowering code for this and don't know what reasoning let to this particular emission, I'd rather not falsely site something due to the later confusion it might cause.  

================
Comment at: test/CodeGen/X86/atomic-non-integer.ll:6
@@ +5,3 @@
+; CHECK: movq	%rdi, %rbx
+; CHECK: callq	__gnu_f2h_ieee
+; CHECK: movw	%ax, (%rbx)
----------------
jfb wrote:
> Why does this involve an f2h conversion? Is it a calling convention thing where half is passed as its integer equivalent? It would be worth documenting, I find it surprising.
Frankly, I have no clue.  We emit the same conversion when doing a non-atomic store, so it's not related to the changes in this patch.  

================
Comment at: test/CodeGen/X86/atomic-non-integer.ll:54
@@ +53,3 @@
+; sanity check the seq_cst lowering since that's the 
+; interesting one from an ordering perspective on x86.
+
----------------
jfb wrote:
> For each of the `xchg` below, can you also `CHECK-NOT: lock` since the x86 manual states that the `lock` prefix is implicit.
This is an implementation detail that is irrelevant to this functionality.  This is a) tested elsewhere, and b) irrelevant to the correctness of the code genation here.  


http://reviews.llvm.org/D15471