[PATCH] D77435: [llvm][CodeGen] Addressing modes for SVE stN.

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes added inline comments.
Herald added a reviewer: ctetreau.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:1417
 
+/// Optimize \param Base and \param  Offset selecting the best addressing mode.
+template <unsigned Scale>
----------------
nit: double space between \param and Offset


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:1425
+  const bool IsRegImm =
+      SelectAddrModeIndexedSVE<-8 /*Min*/, 7 /*Max*/>(N, Base, Base, Offset);
+
----------------
I think comments with var names are usually written like: `/*Min=*/-8, /*Max=*/7`


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:1425
+  const bool IsRegImm =
+      SelectAddrModeIndexedSVE<-8 /*Min*/, 7 /*Max*/>(N, Base, Base, Offset);
+
----------------
c-rhodes wrote:
> I think comments with var names are usually written like: `/*Min=*/-8, /*Max=*/7`
The parameters here are a bit confusing, `N` is passed as `Root` and `Base` for `N` and `Base`? Are both `N` and `Base` necessary or could this be refactored?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:1433
+  // Select the instruction.
+  return (IsRegReg) ? Opc_rr : Opc_ri;
+}
----------------
nit: parentheses are unnecessary


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:3946
       if (VT == MVT::nxv16i8) {
-        SelectPredicatedStore(Node, 2, AArch64::ST2B_IMM);
+        SelectPredicatedStore<0 /*Scale*/>(Node, 2, AArch64::ST2B,
+                                           AArch64::ST2B_IMM);
----------------
as mentioned above: `/*Scale=*/0`


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-stN-reg-imm-addr-mode.ll:7
+; common to all instructions, and varies only for the number of
+; elements of the structure store, which is <N> = 2, 3, 4.
+
----------------
s/structure/structured


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-stN-reg-imm-addr-mode.ll:184
+                                          <vscale x 16 x i8> %v2,
+					  <vscale x 16 x i1> %pred,
+                                          <vscale x 16 x i8>* %base)
----------------
nit: formatting (and below)


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-stN-reg-imm-addr-mode.ll:246
+; CHECK-LABEL: st3b_i8_valid_imm_lower_bound:
+; CHECK:      st3b { z0.b, z1.b, z2.b }, p0, [x0, #-24, mul vl]
+; CHECK-NEXT: ret
----------------
nit: formatting


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77435