[PATCH] D39976: [AArch64] Consider the cost model when folding loads and stores

[Chad Rosier via Phabricator via llvm-commits]

mcrosier added a comment.

The load/store opt pass is already pretty expensive in terms of compile-time.  Did you see any compile-time regressions in your testing?  Also, what performance results have you collected?



================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:592
+static bool isMergeableLdStUpdate(MachineInstr &MI) {
+  // Do update merging. It's simpler to keep this separate from the above
+  // switchs, though not strictly necessary.
----------------
+1 to Florian's comment.  Also, this comment should remain in optimizeBlock.


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:653
+  TargetSchedModel SM;
+  const TargetSubtargetInfo &STI = MF->getSubtarget();
+  SM.init(STI.getSchedModel(), &STI, STI.getInstrInfo());
----------------
You could just add SM and STI to the AArch64LoadStoreOpt class and then initialize it in runOnMachineFunction.


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:682
+  // It is not profitable.
+  else
+    return false;
----------------
You can drop this else.


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1380
                              : getPostIndexedOpcode(I->getOpcode());
+  if (!isProfitableMergeUpdate(NewOpc, *I, *Update))
+    return ++I;
----------------
Add a comment:
  // Evaluate if the new instruction is a better choice than the old ones.


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1381
+  if (!isProfitableMergeUpdate(NewOpc, *I, *Update))
+    return ++I;
+
----------------
I think you can return NextI here.  This will ensure we skip the Update instruction (i.e., the base address add/sub), which is never a candidate for LdStUpdateMerging.


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1408
     DEBUG(dbgs() << "Creating pre-indexed load/store.");
-  else
+  }
+  else {
----------------
Should be
  } else {


Repository:
  rL LLVM

https://reviews.llvm.org/D39976