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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for creating this patch @fpetrogalli!



================
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);
----------------
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.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4439
+    if ((Offset >= Min) && (Offset <= Max)) {
+      Base = N.getOperand(0);
+      OffImm = CurDAG->getTargetConstant(Offset, SDLoc(N), MVT::i64);
----------------
fpetrogalli wrote:
> andwar wrote:
> > `Base` and `OffImm` are only only needed when building in Debug mode and running e.g. `llc` with `-debug`.  Maybe move that inside `LLVM_DEBUG`? 
> No, they are output parameters, passed by reference. They are not just for debug purpose.
nit: unnecessary brackets, can be `if (Offset < Min || Offset > Max)`


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:224
   bool SelectAddrModeFrameIndexSVE(SDValue N, SDValue &Base, SDValue &OffImm);
+  template <int64_t Min, int64_t Max>
+  bool SelectAddrModeIndexedSVE(SDNode *Root, SDValue N, SDValue &Base,
----------------
nit: This one is missing a comment.


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


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4471
+
+  const SDValue ShiftRHS = RHS.getOperand(1);
+  auto *C = dyn_cast<ConstantSDNode>(ShiftRHS);
----------------
nit: Why `const`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74254