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

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally marked an inline comment as done.
cameron.mcinally 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:
> > > 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.
No objections, but I do like Eli's idea for the long term. This would be a step in the right direction for passing fixed-width arguments. Having a restricted IR for fixed-width isn't ideal.


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-subvector.ll:25
+; how fixed length operation are lowered to scalable ones, with multiple blocks
+; ensuring insert/extract sequences are not folded away.
+
----------------
Nit: could probably mark the loads/stores volatile to avoid the branch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82871