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

[Allen zhong via Phabricator via llvm-commits]

Allen marked an inline comment as done.
Allen added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7780
   if (StVT.isScalableVector())
-    report_fatal_error("Cannot scalarize scalable vector stores");
 
----------------
paulwalker-arm wrote:
> Allen wrote:
> > paulwalker-arm wrote:
> > > Allen wrote:
> > > > 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 ?
> > > 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.
> > 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.
> 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.
thanks very much, I revert the change as you said, we do support trunc stores.


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

https://reviews.llvm.org/D123449