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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:86
+  // forward.
+  Optional<MCPhysReg> RenameReg = None;
+
----------------
paquette wrote:
> Is it necessary to initialize this?
I do not think so, but I thought it's worth keeping things explicit.


================
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:
----------------
paquette wrote:
> 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.
yep, mayStore is enough :)


================
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();
----------------
paquette wrote:
> 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?
Yep, an iterator would be the nicest solution here.


================
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>
----------------
paquette wrote:
> 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?
Yep, we exit when the result of Fn evaluates to true. This is a bit weird, but was required to be usable in both cases..... An iterator would be nicer here as well.


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


================
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;
----------------
paquette wrote:
> Instead of using a bool, how about
> 
> ```
> if (any_of(...))
>   break;
> ```
after another look, this is whole loop is not required. I've replaced it with an assert, making sure the register we use for renaming is not trashed.


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


================
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
----------------
paquette wrote:
> Probably better to use update_mir_test_checks.py for this one?
Ah yes! I'll drop the line, I've added the checks manually with additional comments.


================
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
----------------
paquette wrote:
> 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.
Ah yes, I've adjusted the tests to not rely on IR for aliasing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70450