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

[Allen]

Summary

Similar to D122281, we should firstly exclude all scalable vector extending
stores and then selectively enable those which we directly support.

Also merge integer and float scalable vector into scalable_vector_valuetypes.

[paulwalker-arm]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

1184–1185

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?

[Allen]

Merge integer_scalable_vector_valuetypes and fp_scalable_vector_valuetypes into scalable_vector_valuetypes

[paulwalker-arm]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

7780 ↗

(On Diff #421816)

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.

[Allen]

ping?

---

钟云德 Zhong Yunde
Mobile: [phone](For Welink,eSpace Calls)
Email: [e-mail]

发件人：Paul Walker via Phabricator <[e-mail]>
收件人：Zhongyunde <[e-mail]>;paul.walker <[e-mail]>;david.sherwood <[e-mail]>;efriedma <[e-mail]>
抄 送：alextsao1999 <[e-mail]>;llvm-commits <[e-mail]>;kristof.beyls <[e-mail]>;ecnelises <[e-mail]>;acmerzhangxipeng <[e-mail]>;bhuvanendra.kumarn <[e-mail]>;wangyihansh <[e-mail]>;yanliang.mu <[e-mail]>;aeubanks <[e-mail]>;dougpuob <[e-mail]>;michael.hliao <[e-mail]>;mcrosier <[e-mail]>;jun.l <[e-mail]>;diana.picus <[e-mail]>;flo <[e-mail]>;david.green <[e-mail]>;simon.moll <[e-mail]>;ruiling.song <[e-mail]>;caroline.concatto <[e-mail]>
时 间：2022-04-11 18:28:13
主 题：[PATCH] D123449: [AArch64][SelectionDAG] Refactor to support more scalable vector extending stores

paulwalker-arm added inline comments.

Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7780

if (StVT.isScalableVector())

• report_fatal_error("Cannot scalarize scalable vector stores");

---

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.

CHANGES SINCE LAST ACTION

https://reviews.llvm.org/D123449/new/

https://reviews.llvm.org/D123449

[paulwalker-arm]

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

[Allen]

In 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

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

7780 ↗

(On Diff #421816)

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

7780 ↗

(On Diff #421816)

@paulwalker-arm, need I revert this line's change ?

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

1184–1185

Apply you review, thanks!

[paulwalker-arm]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

7780 ↗

(On Diff #421816)

Your setTruncStoreAction related changes are fine. I'm specifically only talking about the single line report_fatal_error("Cannot scalarize scalable vector stores");-> return SDValue(); change. This looks unnecessary and potentially wrong. If I try this patch but with the report_fatal_error left intact I do not see any test failures.

[Allen]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

7780 ↗

(On Diff #421816)

sorry, may be I don't express clear!

I mean that, If I only keep the changes of setTruncStoreAction(VT, InnerVT, Expand) , but without picking out the ones, which are legal, then it will crash at report_fatal_error with above 5 test cases.
So I change report_fatal_error("Cannot scalarize scalable vector stores") into return SDValue() to avoid it.

[paulwalker-arm]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

7780 ↗

(On Diff #421816)

I see, thanks for explaining. I believe in that case the report_fatal_error is correct behaviour. Essentially the code generator has got itself into a state where is needs to scalarise the trunc store (created artificially by pretending we don't support any trunc stores), but scalarisation is not possible with scalable vectors and so we report the error.

If there's a bug then the fix is more likely to be to prevent the call to scalarizeVectorStore. But that's not something you need to worry about for this patch, if at all, because we do support trunc stores.

[Allen]

revert the change of report_fatal_error("Cannot scalarize scalable vector stores") as we do support trunc stores

[Allen]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

7780 ↗

(On Diff #421816)

thanks very much, I revert the change as you said, we do support trunc stores.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

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, MVT::v4i32, MVT::v1i64, MVT::v2i64}) {
       setOperationAction(ISD::MLOAD, VT, Custom);
       setOperationAction(ISD::MSTORE, VT, Custom);
       setOperationAction(ISD::MGATHER, VT, Custom);
       setOperationAction(ISD::MSCATTER, VT, Custom);
     }
 
-    for (MVT VT : MVT::fp_scalable_vector_valuetypes()) {
-      for (MVT InnerVT : MVT::fp_scalable_vector_valuetypes()) {
-        // Avoid marking truncating FP stores as legal to prevent the
-        // DAGCombiner from creating unsupported truncating stores.
+    // Firstly, exclude all scalable vector extending loads/truncating stores,
+    // include both integer and floating scalable vector.
+    for (MVT VT : MVT::scalable_vector_valuetypes()) {
+      for (MVT InnerVT : MVT::scalable_vector_valuetypes()) {
         setTruncStoreAction(VT, InnerVT, Expand);
-        // SVE does not have floating-point extending loads.
-        setLoadExtAction(ISD::SEXTLOAD, VT, InnerVT, Expand);
-        setLoadExtAction(ISD::ZEXTLOAD, VT, InnerVT, Expand);
-        setLoadExtAction(ISD::EXTLOAD, VT, InnerVT, Expand);
-      }
-    }
-
-    // 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()) {
-        // TODO: truncating stores should also be exclude
         setLoadExtAction(ISD::SEXTLOAD, VT, InnerVT, Expand);
         setLoadExtAction(ISD::ZEXTLOAD, VT, InnerVT, Expand);
         setLoadExtAction(ISD::EXTLOAD, VT, InnerVT, Expand);
       }
     }
 
     // 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);
     for (auto Op : {ISD::ZEXTLOAD, ISD::SEXTLOAD, ISD::EXTLOAD}) {
       setLoadExtAction(Op, MVT::nxv2i64, MVT::nxv2i8, Legal);
       setLoadExtAction(Op, MVT::nxv2i64, MVT::nxv2i16, Legal);
       setLoadExtAction(Op, MVT::nxv2i64, MVT::nxv2i32, Legal);
       setLoadExtAction(Op, MVT::nxv4i32, MVT::nxv4i8, Legal);
       setLoadExtAction(Op, MVT::nxv4i32, MVT::nxv4i16, Legal);
       setLoadExtAction(Op, MVT::nxv8i16, MVT::nxv8i8, Legal);
     }