[PATCH] D71215: [AArch64][SVE] Add patterns for unpredicated load/store to frame-indices.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

For every place you're adding `if (scalable) return false;`, I'd like to see a comment explaining why we're bailing out.



================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:1255
       return EVT::getVectorVT(Ty->getContext(), EVT::getEVT(Elm, false),
-                       VTy->getNumElements());
+                       VTy->getElementCount());
     }
----------------
While you're here, indentation?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:6852
+  uint64_t Size = MemVT.isScalableVector() ? MemoryLocation::UnknownSize
+                                           : MemVT.getStoreSize();
+  MachineMemOperand *MMO =
----------------
Should we have a helper for this pattern?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:1326
+    Base = N;
+    OffImm = CurDAG->getTargetConstant(0, dl, MVT::i64);
+    return true;
----------------
This is sort of weird for a method named "SelectAddrModeFrameIndexSVE"; should it not just fail?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:9186
   if (Ty->isSized()) {
-    uint64_t NumBits = DL.getTypeSizeInBits(Ty);
+    uint64_t NumBits = DL.getTypeSizeInBits(Ty).getKnownMinSize();
     NumBytes = NumBits / 8;
----------------
Is this necessary?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:2237
+    // Width = mbytes * elements
+    Scale = 16;
+    Width = SVEMaxBytesPerVector;
----------------
This seems sort of confusing. "Scale" here is implicitly multiplied by vl, and there's isn't any way for the caller to tell except by checking the opcode.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:1165
+
+  let Predicates = [IsLE] in {
+    defm Pat_SpillFill_P16 : spill_fill_predicate<nxv16i1, LDR_PXI, STR_PXI>;
----------------
IsLE?  Are we supposed to do something different on big-endian targets?


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:1178
+
+  defm Pat_ST1B        : unpred_store<nxv16i8, ST1B_IMM, PTRUE_B>;
+  defm Pat_ST1H        : unpred_store<nxv8i16, ST1H_IMM, PTRUE_H>;
----------------
Should we always use PTRUE_B, even for non-byte element sizes, to encourage CSE?

Should we prefer to use ldr/str where legal, to take advantage of the larger immediate offset?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71215