[PATCH] D104217: [AArch64][SVE] Add support for fixed length MSCATTER/MGATHER

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1202
       // NEON doesn't support masked loads or stores, but SVE does
       for (auto VT :
            {MVT::v4f16, MVT::v8f16, MVT::v2f32, MVT::v4f32, MVT::v1f64,
----------------
I realise this has nothing to do with your change, but it looks really odd that we have a nested loop here where we override the VT from the outer loop. Essentially, we're registering the exact same custom operation 4 times in a row! Might be worth fixing at some point.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1203
       for (auto VT :
            {MVT::v4f16, MVT::v8f16, MVT::v2f32, MVT::v4f32, MVT::v1f64,
             MVT::v2f64, MVT::v8i8, MVT::v16i8, MVT::v4i16, MVT::v8i16,
----------------
Not sure if it matters, but isLegalMaskedGatherScatter explicitly excludes MVT::v1f64 as it has < 2 elements.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4313
+  if (IsFixedLength) {
+    IndexVT = getContainerForFixedLengthVector(DAG, IndexVT);
+    MemVT = IndexVT.changeVectorElementType(MemVT.getVectorElementType());
----------------
Is it worth asserting here that `Subtarget->useSVEForFixedLengthVectors()` returns true?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16463
     return performUzpCombine(N, DAG);
+  case AArch64ISD::SETCC_MERGE_ZERO:
+    return performSetccMergeZeroCombine(N, DAG);
----------------
Is there to do this in a separate patch? I think it would be better if possible - I imagine it works as a standalone change where it tidies up the masked load/store tests.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1200
 
       // NEON doesn't support masked loads or stores, but SVE does
       for (auto VT :
----------------
Does the comment need updating to reflect gathers/scatters too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104217