[PATCH] D82871: [SVE] Custom ISel for fixed length extract/insert_subvector.

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm marked 3 inline comments as done.
paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:3244
+// NOTE: When targeting fixed length vectors at SVE the range of MVTs is runtime
+// variable, hence this manual selection.
+static SDNode *extractSubReg(SelectionDAG *DAG, EVT VT, SDValue V) {
----------------
paulwalker-arm wrote:
> efriedma wrote:
> > paulwalker-arm wrote:
> > > efriedma wrote:
> > > > I'm not sure I understand the issue here. Is the problem just that for a pattern, you need to write the type of the result?  I don't think there's any problem with writing a pattern involving a type that isn't always legal; if the type isn't legal, it just won't match.
> > > I believe the issue is the runtime nature of the >128bit fixed length vectors means they are not mapped to any register class, which prevents pattern based matching.
> > > 
> > > We did investigate using hwmodes but it didn't prove to be a viable solution.
> > > I believe the issue is the runtime nature of the >128bit fixed length vectors means they are not mapped to any register class
> > 
> > You could change that in AArch64RegisterInfo.td, if you wanted to.  I don't think that would cause any issues; it's okay if some of the types in the list aren't legal for all subtargets.
> Thanks for the tip, I'll take look and see how it works out.
I tried adding the extra fixed length vector MVTs to ZPRClass but it didn't really work out.

Firstly I needed to fix up a bunch of patterns due to "Could not infer all types in pattern" build errors.  This might be down to ZPRClass being sized as 128, which looks especially weird once it starts to contain a lot of MVTs that are known bigger than 128.  Ultimately though the failing patterns are easily fixed but I guess potentially burdensome since 99.9% of the patterns shouldn't need to care about fixed length SVE.

After this the extract_subvector patterns still do not match.  I've tracked this down to SDTSubVecExtract's usage of SDTCisSubVecOfVec which is not strictly true given the newly expanded definition of extract_subvector.  I cannot just update SDTCisSubVecOfVec because it's used by other operations where we don't want to allow a mixture of fixed length and scalable vectors (e.g. concat_vector).

I can add a new variant of SDTCisSubVecOfVec for use by extract_subvector and insert_subvector, but given the minimal usage I'm not sure it's worth it.  What do you think?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:8678
+  if (InVT.isScalableVector())
+    return Idx == 0 ? Op : SDValue();
+
----------------
efriedma wrote:
> The comment here is really terse, and I'm not sure this is handling all the cases we need to.
> 
> In particular, I'm not sure this correctly handles types like nxv2f32.
I've create a helper function to detect unpacked vector types and used it here so that only fixed length vector extracts from packed scalable types are considered legal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82871