[PATCH] D141431: [AArch64] FEAT_LRCPC3 load/store optimisations

[Sam Elliott via Phabricator via llvm-commits]

lenary added a comment.

Review Notes:

- Post/pre-indexing looks good.
- There's no merging.
- We talked about codegen for the neon instructions in RCPC3. Maybe that should be a different patch when you do implement it?



================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:2535-2537
+    for (const MachineMemOperand *MMO : MI.memoperands())
+      if (!MMO->isUnordered())
+        return false;
----------------
Can you rewrite this with `llvm::any`?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:2540-2542
+  // FIXME temporary to see what this hits besides the instructions we're adding
+  if (MI.hasOrderedMemoryRef())
+    assert(hasRCPC3PrePostIndexVariant(MI));
----------------
I think this fixme is unnecessary, or is development-only.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:2552-2556
+  bool IsImmPreLdSt =
+      IsPreLdSt && MI.getNumOperands() >= 4 && MI.getOperand(3).isImm();
+  bool IsTypicalCase = MI.getNumOperands() >= 3 && MI.getOperand(2).isImm();
 
+  if (!IsTypicalCase && !IsImmPreLdSt && !hasRCPC3PrePostIndexVariant(MI))
----------------
Please split this into three different `if`s with comments to make these conditions readable/explanable. 


================
Comment at: llvm/test/CodeGen/AArch64/aarch64-rcpc3-ldst.ll:10-15
+; TODO:
+; SelDAG implementation
+; merge two ordered load-aqcuires into a load-acquire pair
+; merging of pre/post indexed variants
+; LDIAPP only generated if we have LSE
+; LSE2 makes pair operations single-copy atomic for naturally aligned
----------------
Can you check this TODO?


================
Comment at: llvm/test/CodeGen/AArch64/aarch64-rcpc3-ldst.ll:24-46
+; GISEL-NO-LSE2-LABEL: load_store_2xi32_rcpc3:
+; GISEL-NO-LSE2:    ldapr w10, [x8]
+; GISEL-NO-LSE2:    ldapr w11, [x9]
+; GISEL-NO-LSE2:    stlr w10, [x8]
+; GISEL-NO-LSE2:    stlr w11, [x9]
+;
+; SDAG-NO-LSE2-LABEL: load_store_2xi32_rcpc3:
----------------
The codegen is not different here and is not showing merging. LDIAPP has constraints on when you can merge, which depend on the order of the two accesses, which will need testing.


================
Comment at: llvm/test/CodeGen/AArch64/aarch64-rcpc3-ldst.ll:380-389
+; LDAPUR <Bt>, [<Xn|SP>{, #<simm>}] // Zeroing
+; LDAPUR <Ht>, [<Xn|SP>{, #<simm>}] // Zeroing
+; LDAPUR <St>, [<Xn|SP>{, #<simm>}] // Zeroing
+; LDAPUR <Dt>, [<Xn|SP>{, #<simm>}] // Zeroing
+; LDAPUR <Qt>, [<Xn|SP>{, #<simm>}]
+; STLUR <Bt>, [<Xn|SP>{, #<simm>}]
+; STLUR <Ht>, [<Xn|SP>{, #<simm>}]
----------------
TODO?


================
Comment at: llvm/test/CodeGen/AArch64/aarch64-rcpc3-ldst.ll:395-396
+
+; LDAP1 {<Vt>.D }[index], [<Xn|SP>] // Merging
+; STL1 {<Vt>.D }[index], [<Xn|SP>]
----------------
TODO?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141431