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

[Stelios Ioannou via Phabricator via llvm-commits]

stelios-arm marked 5 inline comments as done.
stelios-arm added inline comments.


================
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,
----------------
dmgreen wrote:
> Should this get some updates to make it more precise for pre-inc pairs?
Could be. Currently, this method is called for load/stores when `getMemOperandWithOffset` returns `true`. However, `getMemOperandWithOffset` has a call to `getMemOpInfo`:

```
  // If this returns false, then it's an instruction we don't want to handle.
  if (!getMemOpInfo(LdSt.getOpcode(), Scale, Width, Dummy1, Dummy2))
    return false;
```
Currently, the `getMemOpInfo` does not include the pre-inc instructions. Additionally, for the post-inc instructions, there is only one variant available for `STRWpost`/`LDRWpost`.  

I am not sure why the other variants are not included, so I am bit skeptical If the pre-inc instructions should be added.

What do you think?

 


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1669
+      // above.
       if (BaseReg == MIBaseReg && ((Offset == MIOffset + OffsetStride) ||
+                                   (Offset + OffsetStride == MIOffset) ||
----------------
dmgreen wrote:
> 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?
For example, the following instructions:

```
str w1 [x0, #20]!
str w2 [x0, #4]
```
Can be paired to:

```
Stp w1, w2, [x0, #20]!
```
The offset of the first and second instruction is `20` and `4`, respectively. The offset stride is `4`. Therefore, the check `(Offset == MIOffset + OffsetStride)` and `(Offset + OffsetStride == MIOffset)` will return `false`.

That’s is why it’s needed. And yes, for such cases it’s only valid for forward now, since the order of the instructions matters for this optimization.


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

https://reviews.llvm.org/D99272