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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm marked 2 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) {
----------------
efriedma wrote:
> paulwalker-arm wrote:
> > 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?
> I guess it's fine to leave as-is, in that case.
Thanks.  I'll circle back round to this after we've enough functionality for fixed length to be usable.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:3385
+
+  case ISD::INSERT_SUBVECTOR: {
+    // Bail when not a "cast" like insert_subvector.
----------------
efriedma wrote:
> paulwalker-arm wrote:
> > efriedma wrote:
> > > There isn't any corresponding code for INSERT_SUBVECTOR in AArch64ISelLowering?
> > Unlike EXTRACT_SUBVECTOR, AArch64ISelLowering didn't have any custom lowering for INSERT_SUBVECTOR for any vector types.  For this reason I assumed the defaults were fine.  My new usage when lowering fixed length to SVE only uses the legal variants so doesn't introduce any new requirements.
> In theory, you need exactly the same code.
> 
> Realistically, I'm not sure target-independent code will actually ever create an insert_subvector with a non-zero index.  I briefly tried grepping through the relevant code, and couldn't find anything. I guess we could ignore the issue for now.
To be consistent I've provided an implementation. We're not really in a position to properly test it, although it defaults to expand for all the cases where tests don't exist.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82871