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

[David Truby via Phabricator via llvm-commits]

DavidTruby added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17212
+  return DAG.getNode(AArch64ISD::SETCC_MERGE_ZERO, DL, CmpVT,
+                         {Pg, Op1, Op2, DAG.getCondCode(ISD::SETNE)});
+}
----------------
Not sure how this got through, I'll re-format in the next revision or before pushing.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17220
+
+  if (Load->getExtensionType() != ISD::LoadExtType::NON_EXTLOAD)
+    return SDValue();
----------------
sdesmalen wrote:
> Are you planning to handle sign/zero-extending loads in a follow-up patch?
Right now they work but perform the extension manually (as you can see in the tests)
The plan is to submit a follow up patch implementing custom lowering for them to use the built in extension in the load instructions


================
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, 
----------------
sdesmalen wrote:
> DavidTruby wrote:
> > sdesmalen wrote:
> > > nit: is MVT::v2f16 missing or is this type widened?
> > I believe v2f16 isn't a legal type here, so we don't want to do the custom lowering for it. We can just allow it to be handled as before.
> Fair enough. Would it be good to at least add a test for it?
I can do, although it's not really testing anything this patch is doing: the <2 x half> seems to be canonicalised before we come through these code paths so I assume that's already tested elsewhere.
Let me know if you want an added test for completeness though :)


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17243
+    if (ContainerVT.isInteger())
+      PassThru = DAG.getConstant(0, DL, ContainerVT);
+    else
----------------
sdesmalen wrote:
> 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.
Initially this is how I had tried to do it but I was getting selection failures because the types weren't quite right, so I decided to just manually create the correct vectors here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101834