[PATCH] D70450: [AArch64] Teach Load/Store optimizier to rename store operands for pairing.

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:86
+  // forward.
+  Optional<MCPhysReg> RenameReg = None;
+
----------------
Is it necessary to initialize this?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:597-607
+static bool isStore(const MachineInstr &MI) {
+  switch (MI.getOpcode()) {
+  default:
+    return false;
+  case AArch64::STRSui:
+  case AArch64::STRQui:
+  case AArch64::STRXui:
----------------
Do you specifically only want the unscaled stores here?

If not, it might make sense to use `AArch64InstrInfo::isPairableLdStInst` along with checking `mayStore`. Then we can avoid duplicating code.


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:818
+  auto MBB = MI.getParent();
+  while (true) {
+    bool isDef = any_of(Cur->operands(), [DefReg, TRI](MachineOperand &MOP) {
----------------
Would it be possible to structure this loop differently so we can avoid `while(true)`?

(Since you're looping backwards, maybe using a `reverse_iterator` would work?)


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:825-827
+    if (isDef || Cur == MBB->begin())
+      break;
+    Cur--;
----------------
Should probably check that `Cur` is valid at some point in this loop?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:832-843
+// Apply Fn to all operands of MI that are physical, non-constant registers.
+static void forAllPhysicalRegs(MachineInstr &MI, const TargetRegisterInfo *TRI,
+                               std::function<void(const MachineOperand)> Fn) {
+  for (ConstMIBundleOperands O(MI); O.isValid(); ++O) {
+    if (!O->isReg())
+      continue;
+    Register Reg = O->getReg();
----------------
This looks very similar to the code in `LiveRegUnits::stepBackward` and `LiveRegUnits::accumulate`.

I think there's a nice opportunity to share some code there. Maybe it would make sense to add a physreg operand iterator or something and use that instead?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:845
+
+// Apply Fn to all sub and super registers of Reg and Reg itself.
+template <typename RetT>
----------------
Is this comment accurate?

Maybe I'm misreading the function here, but it looks like you don't necessarily apply the function to all of the super registers. In the first loop, you can return before hitting the second loop, no?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:860-861
+
+static void updateDefinedRegisters(MachineInstr &MI, LiveRegUnits &Units,
+                                   const TargetRegisterInfo *TRI) {
+
----------------
Would it make sense to move this to `LiveRegUnits`?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:903
+          [OriginalReg, this](MCPhysReg R) -> Optional<MCPhysReg> {
+        if (this->TRI->getMinimalPhysRegClass(OriginalReg) ==
+            this->TRI->getMinimalPhysRegClass(R))
----------------
Do you have to use `this->TRI` in these functions? Can you just pass `TRI` in the capture instead?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:955-961
+      bool isDef =
+          any_of(MI.operands(), [this, RegToRename](MachineOperand &MOP) {
+            return MOP.isReg() && MOP.isDef() &&
+                   this->TRI->regsOverlap(MOP.getReg(), RegToRename);
+          });
+      if (isDef)
+        break;
----------------
Instead of using a bool, how about

```
if (any_of(...))
  break;
```


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1386
+  MachineFunction &MF = *FirstMI.getParent()->getParent();
+  if (RegClass && MF.getRegInfo().tracksLiveness()) {
+    auto RegToRename = getLdStRegOp(FirstMI).getReg();
----------------
This function is pretty long, but if I'm counting braces right, it always returns false when this `if` is false.

I think that something like this might be a bit clearer, and would let you reduce the level of indentation:

```
if (!isStore(FirstMI))
  return false;

MachineFunction &MF = ...
if (!MF.getRegInfo().tracksLiveness())
  return false;

auto *RegClass...
if (!RegClass)
  return false;

...
```


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1409
+    std::function<bool(MachineInstr &, bool)> CheckMIs = [&](MachineInstr &MI,
+                                                             bool isDef) {
+      LLVM_DEBUG(dbgs() << "Checking " << MI << "\n");
----------------
`IsDef`?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1439
+        return true;
+      } else {
+        for (auto &MOP : MI.operands()) {
----------------
Do you actually need the `else` here? I think you should always return in the `if` branch.


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1441
+        for (auto &MOP : MI.operands()) {
+          if (MOP.isReg() && TRI->regsOverlap(MOP.getReg(), RegToRename)) {
+            if (!canRenameMOP(MOP)) {
----------------
You can probably reduce the level of indentation here:

```
if (!MOP.isReg() || !TRI->regsOverlap(...))
  continue;

if (!canRenameMOP(MOP)) {
 ...
}

RequiredClasses.insert(...)
```


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:2085
+
+  LiveRegUnits AllLives;
+
----------------
This doesn't seem to be used anywhere else?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:2132-2135
+  if (MBB.getParent()->getRegInfo().tracksLiveness()) {
+    DefinedInBB.clear();
+    DefinedInBB.addLiveIns(MBB);
+  }
----------------
Might be worth a comment?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:2140-2142
+    if (TII->isPairableLdStInst(*MBBI) && tryToPairLdStInst(MBBI)) {
       Modified = true;
+    } else
----------------
remove braces?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:2186
+
+  // Fn.dump();
 
----------------
remove


================
Comment at: llvm/test/CodeGen/AArch64/stp-opt-with-renaming.mir:1
+# NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+# RUN: llc -start-before=aarch64-ldst-opt -stop-after=aarch64-ldst-opt -o - %s | FileCheck %s
----------------
Probably better to use update_mir_test_checks.py for this one?


================
Comment at: llvm/test/CodeGen/AArch64/stp-opt-with-renaming.mir:2
+# NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+# RUN: llc -start-before=aarch64-ldst-opt -stop-after=aarch64-ldst-opt -o - %s | FileCheck %s
+
----------------
You'll probably want `-verify-machineinstrs` here

Also I think you can just use `-run-pass` instead of `-start-before` and `-start-after` here.


================
Comment at: llvm/test/CodeGen/AArch64/stp-opt-with-renaming.mir:10
+  entry:
+    %dst.0 = getelementptr inbounds i64, i64* %src, i32 4
+    %lv1 = load i64, i64* %src
----------------
fhahn wrote:
> it seems like we need the IR references for the test (AA?). Or is there a trick to get rid of them?
Usually it's because the MIR references some specific bit of the IR.

For example, in this test there's something like this:

`(store 4 into %ir.gxf_entry_el1)`

There are likely more things than this in the test, but I think (if memory serves me well), to deal with *this* specific issue you would just have to replace this MIR line with `(store 4)`. Then you can remove the associated IR because it is no longer referenced.

So theoretically, you can just try deleting all of the IR, see what fails, and then just add back what you need.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70450