[PATCH] D133433: [AArch64-SVE]: lower masked load/store of 128-bit fixed-width vectors
Kerry McLaughlin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 4 09:34:01 PDT 2022
kmclaughlin added inline comments.
================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1397
+ for (MVT VT : MVT::integer_fixedlen_vector_valuetypes()) {
+ if (useSVEForFixedLengthVectorVT(VT,
----------------
david-arm wrote:
> I think at some point we probably want to combine this with the code below:
>
> if (Subtarget->useSVEForFixedLengthVectors()) {
> for (MVT VT : MVT::integer_fixedlen_vector_valuetypes())
> if (useSVEForFixedLengthVectorVT(VT))
> addTypeForFixedLengthSVE(VT);
>
> The problem is that `addTypeForFixedLengthSVE` will add a whole bunch of opcodes all at once, which we're probably not ready for.
>
> @hassnaa-arm perhaps you can simplify this to something like:
>
> if (Subtarget->forceSVEInStreamingMode()) {
> for (MVT VT : MVT::integer_fixedlen_vector_valuetypes())
> if (useSVEForFixedLengthVectorVT(VT, true)
> addTypeForStreamingSVE(VT);
> for (MVT VT : MVT::fp_fixedlen_vector_valuetypes())
> if (useSVEForFixedLengthVectorVT(VT, true)
> addTypeForStreamingSVE(VT);
> }
>
> where you add a function called `addTypeForStreamingSVE` a bit similar to `addTypeForFixedLengthSVE`. For now it would just be:
>
> ```void addTypeForStreamingSVE(EVT VT) {
> setOperationAction(ISD::ANY_EXTEND, VT, Custom);
> setOperationAction(ISD::ZERO_EXTEND, VT, Custom);
> setOperationAction(ISD::SIGN_EXTEND, VT, Custom);
> setOperationAction(ISD::LOAD, VT, Custom);
> setOperationAction(ISD::CONCAT_VECTORS, VT, Custom);
> }```
>
> What do you think?
I also think it would be good to try and simplify this, and for now it might also be worth adding a `TODO` to explain that these functions will be combined once all of the opcodes have been covered?
================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-masked-stores.ll:420
attributes #0 = { "target-features"="+sve" }
+
----------------
nit: extra whitespace :)
================
Comment at: llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-ext-loads.ll:8
+
+define <8 x i16> @load_zext_v16i8i32(<8 x i8>* %ap) #0 {
+; CHECK-LABEL: load_zext_v16i8i32:
----------------
The name of this test doesn't look quite right - I think for this one it should be something like `@load_zext_v8i8i16`, the next one should be `@load_zext_v4i16i32`, etc. I could be wrong, I am just comparing to some of the existing tests we have in `sve-fixed-length-ext-loads.ll`.
================
Comment at: llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-ext-loads.ll:318
+}
+
+
----------------
Is it worth adding tests where the type being extended from is also illegal? Something like this:
```
%a = load <16 x i16>, <16 x i16>* %ap
%val = sext <16 x i16> %a to <16 x i64>
ret <16 x i64> %val
```
================
Comment at: llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-loads.ll:17
+; RUN: llc -aarch64-sve-vector-bits-min=1920 -force-sve-when-streaming-compatible < %s | FileCheck %s -check-prefixes=CHECK,VBITS_GE_1024
+; RUN: llc -aarch64-sve-vector-bits-min=2048 -force-sve-when-streaming-compatible < %s | FileCheck %s -check-prefixes=CHECK,VBITS_GE_2048
+
----------------
There don't seem to be any check lines for any of the `VBITS_GE_*` labels added here? Maybe if they are not needed you could remove the extra labels, or add some check lines to match the ones you need. I think fixing this will also remove the note added at the bottom of this test.
================
Comment at: llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-loads.ll:76
+
+
+attributes #0 = { "target-features"="+sve" }
----------------
Can you please add a test using a load of type which is illegal for Neon, e.g. `32 x float`?
================
Comment at: llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-masked-store.ll:4
+; RUN: llc -aarch64-sve-vector-bits-min=256 -force-sve-when-streaming-compatible < %s | FileCheck %s -check-prefixes=CHECK,VBITS_GE_256_STREAMING
+; RUN: llc -aarch64-sve-vector-bits-min=512 -force-sve-when-streaming-compatible < %s | FileCheck %s -check-prefixes=CHECK,VBITS_GE_512_STREAMING
+
----------------
As with the fixed-length-loads.ll test, I think removing the unused labels here (and in the other test files below) or adding check lines for them will remove the warnings added by the test script.
================
Comment at: llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-masked-store.ll:104
+}
+
+declare void @llvm.masked.store.v16i8(<16 x i8>, <16 x i8>*, i32, <16 x i1>)
----------------
Can you please also add a test with an illegal Neon type?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D133433/new/
https://reviews.llvm.org/D133433
More information about the llvm-commits
mailing list