[PATCH] D74254: [llvm][aarch64] SVE addressing modes.

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2331
+  // Fold (add (vscale * C0), (vscale * C1)) to (vscale * C0 + C1))
+  if (N0.getOpcode() == ISD::VSCALE && N1.getOpcode() == ISD::VSCALE) {
+    APInt C0 = N0->getConstantOperandAPInt(0);
----------------
sdesmalen wrote:
> Does it make sense to submit these DAGCombine changes + all related tests in a separate patch?
> This patch is a bit of three patches in one at the moment: DAGCombines, reg+reg addressing modes, reg+imm addressing modes.
I have extracted the DAGCombiner changes here: https://reviews.llvm.org/D74782

I think that the code for the addressing mode is simple enough that it can stay in a single patch.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4438
+
+  signed Offset = MulImm / MemWidthBytes;
+  if ((Offset < Min) || (Offset > Max))
----------------
sdesmalen wrote:
> Are you purposely using `signed` instead of `int64_t` here?
Should be int64_t for consistency.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4471
+
+  const SDValue ShiftRHS = RHS.getOperand(1);
+  auto *C = dyn_cast<ConstantSDNode>(ShiftRHS);
----------------
sdesmalen wrote:
> nit: Why `const`?
I try to use `const` indiscriminately when I know that the variable is not supposed to change inside the scope. The same it is done in other places in this file (not everywhere). I didn't marry this, so if you want I can remove it. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74254