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

[Dave Green via Phabricator via llvm-commits]

dmgreen accepted this revision.
dmgreen added a comment.
This revision is now accepted and ready to land.

Thanks for adding all these tests. LGTM.



================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:2391
+      (((MI.getOperand(1).isReg() || MI.getOperand(1).isFI()) && !IsPreLdSt) ||
+       ((MI.getOperand(2).isReg() || MI.getOperand(2).isFI()) && IsPreLdSt)) &&
+      "Expected a reg or frame index operand.");
----------------
I think it's probably OK to check
```
assert((MI.getOperand(IsPreLdSt ? 2 : 1).isReg() ||
        MI.getOperand(IsPreLdSt ? 2 : 1).isFI())...
```


================
Comment at: llvm/test/CodeGen/AArch64/ldrpre-ldr-merge.mir:17
+    ; CHECK: liveins: $w0, $w1, $x1
+    ; CHECK: early-clobber $x1, renamable $w0, renamable $w1 = LDPWpre renamable $x1, 5 :: (load 4)
+    ; CHECK: STPWi renamable $w0, renamable $w1, renamable $x1, 0 :: (store 4)
----------------
stelios-arm wrote:
> dmgreen wrote:
> > Although I don't think it's an issue with this patch exactly, should this not have two MMO's? Either be :: (load 8) or :: (load 4) (load 4) ?
> Yes, it should have "(load 4), (load 4)" instead. 
OK. It's because they are identical, so are elided during the merging of memory operands. It's a little strange to not see both, but fine. You don't get any extra information out of having both in this case.


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

https://reviews.llvm.org/D99272