[PATCH] D60156: [X86] Use FILD/FIST to implement i64 atomic load on 32-bit targets with X87, but no SSE2

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Minor comments only.  I'll leave approval to someone more familiar w/x87 if possible



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:27434
+        SDValue Ops[] = { Node->getChain(), Node->getBasePtr() };
+        SDValue Result = DAG.getMemIntrinsicNode(X86ISD::FILD_FLAG,
+                                                 dl, Tys, Ops, MVT::i64,
----------------
You should add a test for a volatile atomic load.  Your code is correct for this case, but it doesn't appear tested.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:27440
+
+        // Now store the X87 register to a stack temporary and convert to i64.
+        SDValue StackPtr = DAG.CreateStackTemporary(MVT::i64);
----------------
The fact that this store *isn't* atomic or volatile deserves a comment.


================
Comment at: llvm/test/CodeGen/X86/atomic-non-integer.ll:232
+; X86-NOSSE-NEXT:    fistpll {{[0-9]+}}(%esp)
+; X86-NOSSE-NEXT:    movl {{[0-9]+}}(%esp), %eax
+; X86-NOSSE-NEXT:    movl {{[0-9]+}}(%esp), %ecx
----------------
If I'm understanding this sequence correctly - not sure I am - it looks like we could simply load directly from your inserted stack temporary instead of shuffling into another and then loading.  Might be worth a TODO


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

https://reviews.llvm.org/D60156