[PATCH] D109827: AArch64: use ldp/stp for 128-bit atomic load/store with v8.4

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

Missing tests?



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:915
+  // v8.4a spec. Unfortunately i128 is not a legal type so the only opportunity
+  // we have to do anything with them is the very first DAG combine.
+  if (Subtarget->hasLSE2()) {
----------------
Why not custom legalize?  Not that it really matters, I guess.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16322
+  MemSDNode *MN = cast<MemSDNode>(N);
+  assert(MN->getAlignment() >= 16 && "ldp only works for aligned addresses");
+
----------------
Assert that we have the expected atomic ordering?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16326
+  SDValue Base, Offset;
+  matchLDPSTPAddrMode(MN->getBasePtr(), Base, Offset, DAG);
+
----------------
Would it make sense to add an AArch64ISD node to use here, instead of doing instruction selection early?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17469
     case AtomicOrdering::Monotonic:
+    case AtomicOrdering::Unordered:
       Opcode = AArch64::CASPX;
----------------
What are you trying to accomplish here?


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

https://reviews.llvm.org/D109827