[PATCH] D123449: [AArch64][SelectionDAG] Refactor to support more scalable vector extending stores

[Allen zhong via Phabricator via llvm-commits]

Allen added a comment.

In D123449#3451986 <https://reviews.llvm.org/D123449#3451986>, @paulwalker-arm wrote:

> Hi @Allen , I think you've missed my previous comment, which is the only thing stopping me from accepting the patch.

hi @paulwalker-arm，I'm sorry to miss that.
 If we revert the following change, then we'll have 5 fail cases. I think none of them are Legal should also be work, as we pick them out are taken into account of the performance.

+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -1214,12 +1214,6 @@ AArch64TargetLowering::AArch64TargetLowering(const TargetMachine &TM,

  }
   
  // Then, selectively enable those which we directly support.

- setTruncStoreAction(MVT::nxv2i64, MVT::nxv2i8, Legal);
- setTruncStoreAction(MVT::nxv2i64, MVT::nxv2i16, Legal);
- setTruncStoreAction(MVT::nxv2i64, MVT::nxv2i32, Legal);
- setTruncStoreAction(MVT::nxv4i32, MVT::nxv4i8, Legal);
- setTruncStoreAction(MVT::nxv4i32, MVT::nxv4i16, Legal);
- setTruncStoreAction(MVT::nxv8i16, MVT::nxv8i8, Legal);

the related failed cases

LLVM :: CodeGen/AArch64/spillfill-sve.ll
 LLVM :: CodeGen/AArch64/sve-forward-st-to-ld.ll
 LLVM :: CodeGen/AArch64/sve-split-store.ll
 LLVM :: CodeGen/AArch64/sve-st1-addressing-mode-reg-reg.ll
 LLVM :: CodeGen/AArch64/sve-trunc.ll



================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7780
   if (StVT.isScalableVector())
-    report_fatal_error("Cannot scalarize scalable vector stores");
 
----------------
paulwalker-arm wrote:
> Thanks for the change @Allen.  Before accepting I just wanted to query this line.  What's the circumstance where it now triggers? (I tried `llvm-lit -v ../llvm-project/llvm/test/CodeGen/AArch64` without this change and nothing jumped out)  I ask because it's not immediate clear to me that the callers are happy for the scalarisation request to be ignored, hence the original error message.
hi @paulwalker-arm，If we revert the following change, then we'll have 5 fail cases. I think none of them are Legal should also be work, as we pick them out are taken into account of the performance.

```
+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -1214,12 +1214,6 @@ AArch64TargetLowering::AArch64TargetLowering(const TargetMachine &TM,
     }
 
     // Then, selectively enable those which we directly support.
-    setTruncStoreAction(MVT::nxv2i64, MVT::nxv2i8, Legal);
-    setTruncStoreAction(MVT::nxv2i64, MVT::nxv2i16, Legal);
-    setTruncStoreAction(MVT::nxv2i64, MVT::nxv2i32, Legal);
-    setTruncStoreAction(MVT::nxv4i32, MVT::nxv4i8, Legal);
-    setTruncStoreAction(MVT::nxv4i32, MVT::nxv4i16, Legal);
-    setTruncStoreAction(MVT::nxv8i16, MVT::nxv8i8, Legal);
```

the related failed cases
```
 LLVM :: CodeGen/AArch64/spillfill-sve.ll
  LLVM :: CodeGen/AArch64/sve-forward-st-to-ld.ll
  LLVM :: CodeGen/AArch64/sve-split-store.ll
  LLVM :: CodeGen/AArch64/sve-st1-addressing-mode-reg-reg.ll
  LLVM :: CodeGen/AArch64/sve-trunc.ll
```


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7780
   if (StVT.isScalableVector())
-    report_fatal_error("Cannot scalarize scalable vector stores");
 
----------------
Allen wrote:
> paulwalker-arm wrote:
> > Thanks for the change @Allen.  Before accepting I just wanted to query this line.  What's the circumstance where it now triggers? (I tried `llvm-lit -v ../llvm-project/llvm/test/CodeGen/AArch64` without this change and nothing jumped out)  I ask because it's not immediate clear to me that the callers are happy for the scalarisation request to be ignored, hence the original error message.
> hi @paulwalker-arm，If we revert the following change, then we'll have 5 fail cases. I think none of them are Legal should also be work, as we pick them out are taken into account of the performance.
> 
> ```
> +++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
> @@ -1214,12 +1214,6 @@ AArch64TargetLowering::AArch64TargetLowering(const TargetMachine &TM,
>      }
>  
>      // Then, selectively enable those which we directly support.
> -    setTruncStoreAction(MVT::nxv2i64, MVT::nxv2i8, Legal);
> -    setTruncStoreAction(MVT::nxv2i64, MVT::nxv2i16, Legal);
> -    setTruncStoreAction(MVT::nxv2i64, MVT::nxv2i32, Legal);
> -    setTruncStoreAction(MVT::nxv4i32, MVT::nxv4i8, Legal);
> -    setTruncStoreAction(MVT::nxv4i32, MVT::nxv4i16, Legal);
> -    setTruncStoreAction(MVT::nxv8i16, MVT::nxv8i8, Legal);
> ```
> 
> the related failed cases
> ```
>  LLVM :: CodeGen/AArch64/spillfill-sve.ll
>   LLVM :: CodeGen/AArch64/sve-forward-st-to-ld.ll
>   LLVM :: CodeGen/AArch64/sve-split-store.ll
>   LLVM :: CodeGen/AArch64/sve-st1-addressing-mode-reg-reg.ll
>   LLVM :: CodeGen/AArch64/sve-trunc.ll
> ```
@paulwalker-arm, need I revert this line's change ?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1218-1219
     // Firstly, exclude all scalable vector extending loads/truncating stores.
     for (MVT VT : MVT::integer_scalable_vector_valuetypes()) {
       for (MVT InnerVT : MVT::integer_scalable_vector_valuetypes()) {
+        setTruncStoreAction(VT, InnerVT, Expand);
----------------
paulwalker-arm wrote:
> Now that we have full coverage, can these iterators be changed to `scalable_vector_valuetypes` and the above `fp_scalable_vector_valuetypes` based loop be deleted?
Apply you review, thanks!


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

https://reviews.llvm.org/D123449