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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Could you please run clang-format on the patch?



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1184
+      // NEON doesn't support masked loads or stores, but SVE does
+      for (auto VT : {MVT::v4f16, MVT::v8f16, MVT::v2f32, MVT::v4f32, MVT::v1f64, MVT::v2f64,
+                      MVT::v8i8, MVT::v16i8, MVT::v4i16, MVT::v8i16, MVT::v2i32, 
----------------
nit: is MVT::v2f16 missing or is this type widened?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4604
+  case ISD::MLOAD:
+    if (useSVEForFixedLengthVectorVT(Op.getValueType()), true)
+      return LowerFixedLengthVectorMLoadToSVE(Op, DAG);
----------------
peterwaller-arm wrote:
> Comma true again.
Are masked SVE masked load/store instructions not always more efficient than scalarising?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17208
+
+static SDValue convertFixedMaskToScalableVector(SDValue Mask, SelectionDAG &DAG) {
+  SDLoc DL(Mask);
----------------
nit: It would be nice if this could be part of `convertToScalableVector`. That way, for //any// fixed-width vector we can ask for its scalable counterpart using the same interface.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17243
+    if (ContainerVT.isInteger())
+      PassThru = DAG.getConstant(0, DL, ContainerVT);
+    else
----------------
This may not be worth it, but just sharing my thoughts; when reading this code I kind of thought "Why not just call `convertToScalableVector`?". This would then end up as a INSERT_SUBVECTOR of a DUP, etc. But that could be simplified by a DAG combine which removes the INSERT_SUBVECTOR.


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-masked-loads.ll:30
+; CHECK: ptrue p{{[0-9]+}}.s, vl2
+; CHECK: fcmeq v[[N:[0-9]+]].2s, v{{[0-9]+}}.2s, v{{[0-9]+}}.2s
+; CHECK: ld1w { z0.s }, p[[N]]/z, [x0]
----------------
Please use `CHECK-NEXT` for these, so that we can check there are no other instructions being emitted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101834