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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


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


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:6852
+  uint64_t Size = MemVT.isScalableVector() ? MemoryLocation::UnknownSize
+                                           : MemVT.getStoreSize();
+  MachineMemOperand *MMO =
----------------
efriedma wrote:
> Should we have a helper for this pattern?
Yes, that would be useful. I've added `MemoryLocation::getSizeOrUnknown(const TypeSize &)`


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


================
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;
----------------
efriedma wrote:
> Is this necessary?
No, good catch!


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:2237
+    // Width = mbytes * elements
+    Scale = 16;
+    Width = SVEMaxBytesPerVector;
----------------
efriedma wrote:
> 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.
I'm not sure if is an actual issue in practice though. Are you suggesting to make Scale a `TypeSize` instead of an `unsigned`?


================
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>;
----------------
efriedma wrote:
> IsLE?  Are we supposed to do something different on big-endian targets?
No, that was a misunderstanding on my part. I've removed this now.


================
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>;
----------------
efriedma wrote:
> 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?
> Should we always use PTRUE_B, even for non-byte element sizes, to encourage CSE?
Our experience is that vectorized loops have most predicates CSEd anyway. For a loop that operates on two lanes, often a predicate is already available and there is no need to introduce an extra `ptrue_b`. If a loop using floats is vectorized with VF=2, we don't want operations on `<vscale x 2 x float>` to use `ptrue.b` because that would enable operations on all (vscale x) 4 lanes, which may not be valid.

> Should we prefer to use ldr/str where legal, to take advantage of the larger immediate offset?
That would not be endian safe, hence the preference to use ST1 (note that the order is dictated by the AAPCS for when passing the vectors by reference). This case of saving/restoring to/from the stack like this is pretty rare. Normal spills and fills will indeed use the STR/LDR instructions. And normal load/store vector instructions that are not storing to a local will likely use other addressing modes like reg+reg.


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

https://reviews.llvm.org/D71215