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

[JF Bastien via llvm-commits]

jfb added a comment.

In http://reviews.llvm.org/D15471#311072, @reames wrote:

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


OK ty.

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


Oh yeah I just want to know that we generate something. If it's wrong the fix should definitely be separate.

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


Yes.

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


I want to improve our test coverage and understand what the tests are doing. I agree that lowering improvements are separate.


================
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
+
----------------
reames wrote:
> 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.  
Could you document this at the top of the test? It's not clear from the name of the test that you're not checking correctness of the generated code.

================
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)
----------------
reames wrote:
> 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.  
I'd rather know if the test is correct, and fix elsewhere if not: otherwise it's hard to reason about this test when fixing bugs.

I can't find anything about `half` or `fp16` in the [calling convention](http://www.x86-64.org/documentation/abi.pdf), but the signature (`short __gnu_f2h_ieee(float)`) leads me to believe that the ABI passes things as `float` and then converts to `short` as a proxy for `half`.

I think a cleaner test wouldn't pass a `half` in registers but would instead pass two pointers. That makes the test way easier to understand IMO.

================
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.
+
----------------
reames wrote:
> 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.  
Oh you're right, `test/CodeGen/X86/atomic_mi.ll` tests this and `git blame` shows a familiar name on that!


http://reviews.llvm.org/D15471