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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
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, 
----------------
DavidTruby wrote:
> 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 :)
> Let me know if you want an added test for completeness though :)
That would be good. I'm aware your patch isn't really doing anything specific for that type and I would suspect it all just works, but to confirm that I would have to download the patch, build it, and try it out, whereas it could also be confirmed (and guarded) by an extra test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101834