[PATCH] D101834: [llvm][sve] Lowering for VLS MLOAD/MSTORE

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm added a comment.

Disclosure: I had a hand in writing an early draft of a small part of this, so should definitely not be the only reviewer.

Early comments on first pass through.

I wonder for a moment: given that the "if condition"s are wrong in the LowerOperation switch -- and the tests didn't catch it means that we should have a test which would have caught it. Not sure what that would look like at the moment beyond it would need to be a test for non-scalable vector type.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4476
   case ISD::CONCAT_VECTORS:
+
     return LowerCONCAT_VECTORS(Op, DAG);
----------------
Nit: Extraneous blank.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4557
+  case ISD::MSTORE:
+    if (useSVEForFixedLengthVectorVT(cast<MaskedStoreSDNode>(Op)->getValue().getValueType()), true)
+      return LowerFixedLengthVectorMStoreToSVE(Op, DAG);
----------------
Suggestion to avoid the cast.

Also, looks like you're using the "comma true" operator in an if, which I'm guessing is unintended?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4604
+  case ISD::MLOAD:
+    if (useSVEForFixedLengthVectorVT(Op.getValueType()), true)
+      return LowerFixedLengthVectorMLoadToSVE(Op, DAG);
----------------
Comma true again.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17207
 
+
+static SDValue convertFixedMaskToScalableVector(SDValue Mask, SelectionDAG &DAG) {
----------------
Nit: Extraneous blank


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h:230
 
+    // For fixed vectors, aqvoid scalarization if using SVE for them.
+    if (isa<FixedVectorType>(DataType) && !ST->useSVEForFixedLengthVectors())
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101834