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

[Tim Northover via Phabricator via llvm-commits]

t.p.northover marked 4 inline comments as done.
t.p.northover added inline comments.


================
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()) {
----------------
efriedma wrote:
> Why not custom legalize?  Not that it really matters, I guess.
It gets converted to a `cmpxchg` during `LegalizeTypes`, and I don't think we can hook in there.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16326
+  SDValue Base, Offset;
+  matchLDPSTPAddrMode(MN->getBasePtr(), Base, Offset, DAG);
+
----------------
efriedma wrote:
> Would it make sense to add an AArch64ISD node to use here, instead of doing instruction selection early?
Good idea. That should let us use `SelectAddrMode...` from ISelDAGToDAG instead of reinventing it here.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17469
     case AtomicOrdering::Monotonic:
+    case AtomicOrdering::Unordered:
       Opcode = AArch64::CASPX;
----------------
efriedma wrote:
> What are you trying to accomplish here?
Ah, I've got a separate patch to use `CASP` if LSE is present but not this LSE2. They started off together and I think this is a remnant of that bit. I'll remove it.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp:1029
+  MachineOperand &RHSOp1 = RHS->getOperand(1);
+  if (!RHSOp1.isCImm() || RHSOp1.getCImm()->getBitWidth() > 64)
+    return;
----------------
paquette wrote:
> I think `mi_match` could really simplify this function.
> 
> ```
> if (mi_match(Root, MRI, m_GPtrAdd(m_Reg(NewBase), m_ICst(NewOffset))) && isShiftedInt<7, 3>(NewOffset))) {
>   Base = NewBase;
>   Offset = NewOffset;
> }
> ```
> 
> ... But I'm not sure about the >64 bit widths.
Oh, neat! Didn't know that existed.


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

https://reviews.llvm.org/D109827