[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