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

[Bradley Smith via Phabricator via llvm-commits]

bsmith added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4322
+    }
+  }
+
----------------
peterwaller-arm wrote:
> 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.
This makes sense for MGATHER since the passthru value is totally ignored, however for masked_load we don't have target specific isd nodes, hence the passthru value makes it down to selection.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4434
+  if (IsFixedLength) {
+    if (Index.getSimpleValueType().isFixedLengthVector())
+      Index = convertToScalableVector(DAG, IndexVT, Index);
----------------
peterwaller-arm wrote:
> 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?
`selectGatherScatterAddrMode` can swap the BasePtr/Index operands, one of which is a scalar, the other a vector. Hence here we don't know which is which, so we check them both.


================
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;
----------------
peterwaller-arm wrote:
> This was changed in D101834 so it looks like you need a rebase since the MLOAD work.
It was `isLegalMaskedLoadStore` that was changed in D101834, not this function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104217