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

[Stelios Ioannou via Phabricator via llvm-commits]

stelios-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:2257
   // Make sure this is a reg/fi+imm (as opposed to an address reloc).
   assert((MI.getOperand(1).isReg() || MI.getOperand(1).isFI()) &&
          "Expected a reg or frame index operand.");
----------------
dmgreen wrote:
> Is this assert valid/sensible for PreInc?
The assert is valid for pre-inc ld/st because `MI.getOperand(1)` is the destination register.


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1044
+  // Adds the pre-index operand for pre-indexed ld/st pairs.
+  if (isPreLdSt(*RtMI) && (getPreIndexedOpcode(Rt2MI->getOpcode()) == Opc))
+    MIB.addReg(BaseRegOp.getReg(), RegState::Define);
----------------
dmgreen wrote:
> Does this need the second condition in the if? Should that always be true if the first part is a pre?
If it reaches at this point, the second condition is redundant. Therefore, I will remove it.  


================
Comment at: llvm/test/CodeGen/AArch64/strpre-str-merge.mir:357
+    early-clobber renamable $x0 = STRSpre killed renamable $s0, killed renamable $x0, 12 :: (store 4)
+    early-clobber renamable $x0 = STRSpre killed renamable $s1, killed renamable $x0, 12 :: (store 4)
+    RET undef $lr, implicit $x0
----------------
dmgreen wrote:
> What happens if this is 16? (Or 4)
It exhibits the same behaviour. 


================
Comment at: llvm/test/CodeGen/AArch64/strpre-str-merge.mir:357
+    early-clobber renamable $x0 = STRSpre killed renamable $s0, killed renamable $x0, 12 :: (store 4)
+    early-clobber renamable $x0 = STRSpre killed renamable $s1, killed renamable $x0, 12 :: (store 4)
+    RET undef $lr, implicit $x0
----------------
stelios-arm wrote:
> dmgreen wrote:
> > What happens if this is 16? (Or 4)
> It exhibits the same behaviour. 
They will not be merged.


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

https://reviews.llvm.org/D99272