[PATCH] D133433: [AArch64-SVE]: lower all types of loads and stores of fixed-width vector

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1398
+    if (Subtarget->forceSVEInStreamingMode()) {
+      for (MVT VT : MVT::integer_fixedlen_vector_valuetypes()) {
+        if (useSVEForFixedLengthVectorVT(VT, true)) {
----------------
nit: unnecessary curly braces here and below.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12404
+    if (VT.isScalableVector() ||
+        useSVEForFixedLengthVectorVT(VT, Subtarget->forceSVEInStreamingMode()))
       return LowerToPredicatedOp(Op, DAG, AArch64ISD::SHL_PRED);
----------------
Are you doing these as part of the same patch, because tests like `sve-fixed-length-int-shifts.ll` require loads and stores to work?

I wonder if you can still split out the unpredicated, non-extending/truncating loads/stores from this patch, such that you can have:

1. patch for basic loads/stores + anything required to make these work (including tests)
2. patches for shifts, concat_vectors, build_vector, vector_shuffle, extract_vector_elt, extract_subvector (or at least as many that are not required for (1)), with corresponding tests (e.g. `sve-fixed-length-int-shifts.ll` -> `sve-streaming-compatible-fixed-length-int-shifts.ll`), because these tests are currently missing.
3. patch for masked and extending/truncating loads and stores.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:3036
+  let Predicates = [IsForcingSVEDisabled] in {
+    let AddedComplexity = 1 in {
+    def : Pat<(i32 (vector_extract (nxv16i8 ZPR:$vec), VectorIndexB:$index)),
----------------
I think you can just write:

  let AddedComplexity = 1, Predicates = [IsForcingSVEDisabled] in {
    ...
  }

which would avoid the extra indentation.


================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.cpp:69
+static cl::opt<bool>
+    ForceSVEWhenStreamingCompatible("force-sve-when-streaming-compatible",
+                                    cl::init(false), cl::Hidden);
----------------
Can you rename this variable to `ForceStreamingCompatibleSVE`? (likewise change the name of the flag to `-force-streaming-compatible-sve`)

The current name `ForceSVEWhenStreamingCompatible` suggests to use the full range of SVE instructions when in streaming-compatible mode, even the instructions that would be illegal in that mode, but that would be incorrect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133433