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

[JF Bastien via llvm-commits]

jfb added a comment.

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.

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?).


================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:207
@@ +206,3 @@
+
+/// Convert an atomic load of a non-integeral type to an integer load of the
+/// equivelent bitwidth.  See the function comment on
----------------
"integral"

================
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
----------------
"integral"

================
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
+
----------------
Add a reference to:
Also add a reference to: https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html

================
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)
----------------
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.

================
Comment at: test/CodeGen/X86/atomic-non-integer.ll:32
@@ +31,3 @@
+; CHECK: movzwl	%ax, %edi
+; CHECK: jmp	__gnu_h2f_ieee
+  %v = load atomic half, half* %fptr unordered, align 2
----------------
Same.

================
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.
+
----------------
For each of the `xchg` below, can you also `CHECK-NOT: lock` since the x86 manual states that the `lock` prefix is implicit.

================
Comment at: test/Transforms/AtomicExpand/X86/expand-atomic-non-integer.ll:5
@@ +4,3 @@
+; `llvm::convertAtomicStoreToIntegerType`. If X86 stops using this 
+; functionality, please move this test to a target which still is.
+
----------------
Also test `volatile` and `addressspace` combinations.


http://reviews.llvm.org/D15471