[PATCH] D135324: [AArch64-SVE]: force using SVE in streaming mode to lower arithmetic and logical fixed-width vector ops.

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1396
       for (MVT VT : {MVT::v8i8, MVT::v16i8, MVT::v4i16, MVT::v8i16, MVT::v2i32,
-                     MVT::v4i32, MVT::v2i64})
+                     MVT::v4i32, MVT::v1i64, MVT::v2i64})
         addTypeForStreamingSVE(VT);
----------------
sdesmalen wrote:
> paulwalker-arm wrote:
> > hassnaa-arm wrote:
> > > david-arm wrote:
> > > > I remember in one of your previous patches that @sdesmalen mentioned you shouldn't need to add `v1i64` as it should be treated as a scalar. What happens if you remove it? I imagine your v1i64 tests might just generate scalar code?
> > > Yes, at that patch, there were no tests needing custom-lowering for v1i64.
> > > 
> > > But in this patch, the test file of sve-streaming-mode-fixed-length-int-log.ll 
> > > has invalid instructions for the test cases of : 
> > > ```
> > > define <1 x i64> @and_v1i64(<1 x i64> %op1, <1 x i64> %op2)
> > > ```
> > > ```
> > > define <1 x i64> @xor_v1i64(<1 x i64> %op1, <1 x i64> %op2)
> > > ```
> > > 
> > I'll also add that in general you don't want to revert such integer types to scalar because that'll cause GPR-VPR transfers that can be expensive, perhaps even more so when it comes to streaming mode.  You can also see that within `LowerMUL` we have special handling for `v1i64` to keep it in the vector unit.
> Does that mean we need coverage as well for v1i32?  (and perhaps also v1i8, v1i16). If so, I wonder if that might warrant a separate patch rather than support the odd case here?
Discussed offline but for completeness the answer is no. The other MVTs you list are not type legal (only 64/128-bit vectors are legal for NEON) and so they'll not make it into operation legalisation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135324