[PATCH] D153972: [AArch64] Fold tree of offset loads combine

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Yeah, I am totally onboard with the motivation of this work. We have seen this come up as a problem a few times and it is easy to see from the test codegen changes that this will give some decent improvements. The large combine and the amount of code is a little bit unfortunate, but I guess it is what it is.

The usual testing question because this is not visible in these diffs: are the tests with volatile loads and atomics in place where this shouldn't trigger?

I have just done a first pass over this code and left nits inlined. Will do a second pass tomorrow.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18375
+  if (auto *Ld = dyn_cast<LoadSDNode>(BV)) {
+    if (!Ld || !Ld->isSimple() || !BV.hasOneUse())
+      return false;
----------------
Nit: `hasOneUse` is already checked above. 


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18406
+        B.getOperand(1).getOpcode() != ISD::CONCAT_VECTORS ||
+        B.getOperand(1).getNumOperands() != 4)
+      return false;
----------------
Not sure if it deserve a comment, but why the 4 here?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18426
+    auto *Ld0 =
+        dyn_cast<LoadSDNode>(B.getOperand(0).getOperand(0).getOperand(0));
+    auto *Ld1 =
----------------
Nit: `.getOperand(0).getOperand(0).getOperand(0)` is a bit cryptic, perhaps a pointer to it helps.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18462
+
+  if (Op0.getOpcode() != Op1.getOpcode() || !Op0.hasOneUse() ||
+      !Op1.hasOneUse())
----------------
Nit: this can be checked first in the function as an early exit before doing more work in `isLoadOrMultipleLoads`? 


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18527
+
+  // Attempt to rule out some unprofitable cases using heiristics (some working
+  // around suboptimal code generation), notably if the extend not be able to
----------------
typo


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

https://reviews.llvm.org/D153972