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

[David Truby via Phabricator via llvm-commits]

DavidTruby added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:7577-7584
+  if (isFixedSVE) {
+    assert(isTypeLegal(VT) && "Expected only legal fixed-width types");
+    VT = getContainerForFixedLengthVector(DAG, VT);
+    IntVT = getContainerForFixedLengthVector(DAG, IntVT);
+
+    In1 = convertToScalableVector(DAG, VT, In1);
+    In2 = convertToScalableVector(DAG, VT, In2);
----------------
paulwalker-arm wrote:
> This doesn't look safe with respect to the extend/rounding code just below.  When faced with differing types the result from both convertToScalableVector called will be a type of the same size.  However their element counts will be different.  For example take the case:
>  ```
> fcopysign v8f64, v8f32
> ```
> this will resulting in:
> ```
> In1 = nxv2f64
> In2 = nxv4f32
> ```
> which I doubt the remaining logic will handle properly.  The most likely affect being a getNode assert firing for invalid operands.
> 
> My guess is that you're not seeing this because `In1` and `In2` always have the same type and indeed I couldn't immediate see a way to exercise this logic.  I think this means your "mixtype" tests are likely exercising nothing new and are redundant.  This is likely also true for you original patch when you added the initial scalable vector support.  If they are not exercising this code as I suspect then you either need to rewrite them or just remove them if there's no actually route to test this logic.
> 
> Personally I think the safest route is to simply rewrite the fixed length fcopysign into a scalable vector one after any necessary extending/rounding of the input has taken place.
> 
> For what it's worth I also think the use of FP_EXTEND/FP_ROUND is not the most efficient way to get the sign bits to align but that can be changed later.
I believe I've corrected this now; I think you're right that the inputs will always be the same type anyway though. I agree that it is safer to leave the handling in just in case that does get triggered.
I think it's better to leave the mixed type tests in as is, just in case something changes in future and the types coming into this function could be different we want to make sure we don't regress in that case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128642