[PATCH] D99272: [AArch64] Adds a pre-indexed paired Load/Store optimization for LDR-STR.

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

It would be good to see some extra tests for various edge cases, like offsets near to the boundaries and different pairs of instructions being combined/not.



================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:2238
+  // to ldp q0, q1, [x11, #32]!
   if (MI.getOperand(1).isReg()) {
     Register BaseReg = MI.getOperand(1).getReg();
----------------
stelios-arm wrote:
> Note for `LDRQpre` instructions it should be `MI.getOperand(2).getReg()`, and also `BaseReg` should be `Register BaseReg = MI.getOperand(2).getReg()`. This can be easily fixed, however it won’t do much because `MI.modifiesRegister(BaseReg, TRI)` will again result to true.  Any suggestions? 
The IsPreLd can move to the outer if, and can be IsPreSt too? The register it is using isn't correct, but it's unpredictable for a writeback load/store to load/store the same register as the operand. So the check is OK to skip for preinc loads/stores.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1972
 
 bool AArch64InstrInfo::isUnscaledLdSt(unsigned Opc) {
   switch (Opc) {
----------------
I feel like "Unscaled" instructions are a set of instructions on their own right. Can we rename the function to something like hasUnscaledLdStOffset to make it clear what it means now?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:2820
 /// Only called for LdSt for which getMemOperandWithOffset returns true.
 bool AArch64InstrInfo::shouldClusterMemOps(
     ArrayRef<const MachineOperand *> BaseOps1,
----------------
Should this get some updates to make it more precise for pre-inc pairs?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:572
 
+static bool isPairedPreLdSt(unsigned int Opc) {
+  switch (Opc) {
----------------
This is only used in one place, where it could be checking isPreLdSt on the original opcode?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:675
+  if (IsPreLdSt)
+    PairedRegOp = 1;
+  unsigned Idx = isPairedLdSt(MI) || IsPreLdSt ? PairedRegOp : 0;
----------------
+= 1?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1649
+      // destination register it can’t be paired with a pre-index ld/st
+      // pair. Additionaly if the base reg is used or modified the operatons
+      // can't be paired: bail and keep looking.
----------------
-> Additionally
-> operations


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1669
+      // above.
       if (BaseReg == MIBaseReg && ((Offset == MIOffset + OffsetStride) ||
+                                   (Offset + OffsetStride == MIOffset) ||
----------------
Why does this not already handle the combining of PreLdStPair?

The existing code can combine in both directions. Presumably it's only valid for forward now?


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

https://reviews.llvm.org/D99272