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

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm added a comment.

Mostly nits -- looks like it needs a rebase.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4230
+  // question will have fixed<->scalable conversions around them. This should be
+  // moved to a DAG combine or complex pattern, after all of the fixed vector
+  // insert and extracts have been removed.
----------------
Is it intended to address this in the near future?

Nit: I initially read 'the code should be moved, once <condition>', but I think you mean, 'the code should be moved, so that it executes after <condition>'.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4315
+  } else {
+    if (isZerosVector(PassThru.getNode())) {
+      if (IndexVT.isInteger())
----------------
Nit: `} else if` and remove a level of indent.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4322
+    }
+  }
+
----------------
I wonder if the undef-or-zero logic can be cleanly combined into a function with MLOAD. But maybe not. I wouldn't wonder for too long.

Thought (1): is the PassThru value actually needed for the Ops, or could it use a constant there? My thinking is that the underlying operation is invariant of its PassThru operand (hence the need for the select).

If (1) is right, then could you use an empty SDValue, which gets assigned to the thing to be 'selected', and the logic becomes `if (PassThru) { insert select }`.

I think this is a matter of taste so feel at liberty to ignore the suggestion. If you go for it please update MLOAD.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4434
+  if (IsFixedLength) {
+    if (Index.getSimpleValueType().isFixedLengthVector())
+      Index = convertToScalableVector(DAG, IndexVT, Index);
----------------
Pondering: Why are these two isFixedLengthVector queries needed? I read: you have a fixed-vector operation, and it has scalable operands? Can that arise? How so?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h:246
   bool isLegalMaskedGatherScatter(Type *DataType) const {
-    if (isa<FixedVectorType>(DataType) || !ST->hasSVE())
+    if (!ST->hasSVE())
       return false;
----------------
This was changed in D101834 so it looks like you need a rebase since the MLOAD work.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h:249
 
+    // For fixed vectors, avoid scalarization if using SVE for them.
+    auto *DataTypeFVTy = dyn_cast<FixedVectorType>(DataType);
----------------
Nit: Potentially confusing comment about the mechanism of the following block, because it's written in actively: "this block avoids scalarization", but actually the only thing the block can do is return false, which causes scalarization. Suggested form: "Fixed-length vectors do not use masked gather/scatter unless ..."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104217