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

[Andrzej Warzynski via Phabricator via llvm-commits]

andwar added a comment.

Hi @fpetrogalli, thank you for working on this.

- IMHO every test functions (e.g.:`test_masked_ldst_sv2i8 `) should either test contiguous **load or store** (i.e. only one thing at a time). That will help triaging potential bugs in the future and also would be consistent with other test files in this folder.
- [nit] AFAIK, `+` is never used in file names (e.g. //sve-pred-non-temporal-ldst-addressing-mode-reg+reg.ll//).  I couldn't find any specific rule for that though.
- [nit] The idea of `%sv2i1  = type <vscale x 2 x i1>` is very neat, but I'm slightly worried that that will complicate searching through test files (we are quite familiar with `<vscale x 2 x i1>`, not so much with `%sv2i1`, so you might miss cases when using `grep`).
- Are the DAG Combine rules required for this patch? If so, could you add tests for them?



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4420
+                                                   SDValue &OffImm) {
+  if (!isa<MemSDNode>(Root))
+    return false;
----------------
[nit] Wouldn't assert be more suitable?


================
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);
----------------
`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`? 


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4441
+      OffImm = CurDAG->getTargetConstant(Offset, SDLoc(N), MVT::i64);
+      LLVM_DEBUG(dbgs() << "Match found:\n"; dbgs() << "ROOT:\n"; Root->dumpr();
+                 dbgs() << "BASE:\n"; Base.dumpr(); dbgs() << "OFFSET:\n";
----------------
[nit] `Match found` is very generic (and you use it twice). Maybe sth more specific?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4454
+                                                  SDValue &Offset) {
+  const unsigned Opcode = N.getOpcode();
+  const SDLoc dl(N);
----------------
Used only once (so no need for a dedicated variable)


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4455
+  const unsigned Opcode = N.getOpcode();
+  const SDLoc dl(N);
+
----------------
Not used.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4464
+
+  // We don't match addition to constants
+  if (isa<ConstantSDNode>(RHS) || isa<ConstantSDNode>(LHS))
----------------
Hm, why? Maybe document this method like you did for `SelectAddrModeIndexedSVE`?


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:6979
+/// Addressing modes
+def am_sve_indexed_s4 :ComplexPattern<i64, 2, "SelectAddrModeIndexedSVE<-8,7>", [], [SDNPWantRoot]>;
+
----------------
Inconsistent naming with the records that follow this one.


================
Comment at: llvm/test/CodeGen/AArch64/sve-gep.ll:7
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    rdvl x8, #1
-; CHECK-NEXT:    add x0, x0, x8, lsl #2
+; CHECK-NEXT:    rdvl x8, #4
+; CHECK-NEXT:    add x0, x0, x8
----------------
Unrelated changes.


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

https://reviews.llvm.org/D74254