[PATCH] D128642: [AArch64][SVE] Use SVE for VLS fcopysign for wide vectors

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:3597
+
+  if (!isTypeLegal(N->getValueType(0)))
+    return DAG.UnrollVectorOp(N, N->getValueType(0).getVectorNumElements());
----------------
DavidTruby wrote:
> paulwalker-arm wrote:
> > By this point we know the result type is legal because results are legalised before operands.  What's important here is the result type remains legal after splitting the operands.  Given the result and first operands have the same type this means ensuring the types of `LHSLo` and `LHSHi` are legal after splitting.
> > 
> > There's a function `GetSplitDestVTs` which returns the types expected from splitting.  I mention this because I think it's better to query the expected types are legal before performing the actual splitting. 
> Ah ok I think I was considering this wrong, I thought that the result type of the concat (which is the result type of the original FCOPYSIGN) needed to be legal for us to do the transform
> 
> If that's already legal, is there a problem? Is there a case where splitting an already legal vector in two would make a vector illegal? (genuine question I'm not sure when this would pop up)
> 
> Or do we need RHSLo to be legal? 
You can have multiple legal types for the same vector element type.  For NEON `v4f32` and `v2f32` are legal.  So it is possible for the result type to be legal and yet still be legal after splitting.  Likewise `v1f32` is not legal for NEON and so it is possible to enter with a legal type that would become illegal when split.

For the former case we can split the operation in two as you've done.  For the latter we're better reverting to the original code path of calling `UnrollVector`. So generally what you've done is fine, it is just you're checking the wrong type (i.e. N's result type rather than the expected result type of the new `FCOPYSIGN` operations).  Plus my comment that you probably want to use `GetSplitDestVTs` so you only call `SplitVector` for the cases that are safe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128642