[PATCH] D80384: [SVE] Add flag to specify SVE register size, using this to calculate legal vector types.

[Paul Walker via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3500
+  if (VT.getVectorElementType() == MVT::i1)
+    return false;
+
----------------
efriedma wrote:
> Restricting i1 types like this seems a little weird to me.  We don't need to pretend to be NEON. We want these types to be legal for similar reasons we want the other fixed vector types to be legal.  I guess we could leave the check for now if it makes it easier to get simple testcases working, but I don't think this is what we want long-term.
> 
> For non-i1 types, should we check the element type is legal?  integer_fixedlen_vector_valuetypes() and fp_fixedlen_vector_valuetypes() can return some exotic types.
Part of this is me not looking for trouble at this stage and wanting to wait to see if there's a performance reason to make them legal.  I think the majority of the cases can be handled after the conversion to scalable vector types, which just leaves the cross-block scenarios.

However, from a functional point of view I'm trying not to make any ABI related changes. Currently the handling of i1 based vectors (well all fixed length vectors) has already been decided for NEON and so I cannot just change it in isolation without breaking compatibility with existing libraries (which don't know about SVE).

In the future we'll need a function attribute that corresponds to an as yet undefined ABI. When that is in place we can relax the i1 restriction.  For now though my focus is purely on block level code generation.



================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.cpp:345
+
+unsigned AArch64Subtarget::getMaxSVEVectorSizeInBits() const {
+  assert(HasSVE && "Tried to get SVE vector length without SVE support!");
----------------
efriedma wrote:
> We probably want function attributes?
> 
> It looks like this patch doesn't use the max size; what do you anticipate using it for?
When creating the patch I did spot "min-legal-vector-width" exists as a function attribute but as mentioned above I'm trying to avoid establishing anything ABI related at this stage.

As part of D80385 where I've added the createPredicateForFixedVector function there's a comment that mentions the use case for the max size.  Essentially when min==max we can avoid the need to create explicitly sized predicates and instead use "PTRUE ALL" as a route to select unpredicated instructions when available.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80384