[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/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);
----------------
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.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18968-18970
-  // Don't expand for SVE2
-  if (!VT.isScalableVector() || Subtarget->hasSVE2() || Subtarget->hasSME())
     return SDValue();
----------------
Isn't this original code now fine and you instead just need to remove the following 
```
  // Don't expand for NEON
  if (VT.isFixedLengthVector())
    return SDValue();
```
block because that is covered by the `!VT.isScalableVector()` check?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128642