[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/DAGCombiner.cpp:138
 
+static cl::opt<bool> EnableVectorFcopysignExtendRound(
+  "combiner-vector-fcopysign-extend-round", cl::Hidden, cl::init(false),
----------------
Up to you but I think `EnableVectorFCopySignExtendRound` looks better.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:140
+  "combiner-vector-fcopysign-extend-round", cl::Hidden, cl::init(false),
+  cl::desc("Enable merging extends and rounds into FCOPYSIGN on vector types"));
+
----------------
for?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:3612
+  SDValue Lo =
+      DAG.getNode(ISD::FCOPYSIGN, DL, LHSLo.getValueType(), LHSLo, RHSLo);
+  SDValue Hi =
----------------
LHSLoVT?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:3614
+  SDValue Hi =
+      DAG.getNode(ISD::FCOPYSIGN, DL, LHSHi.getValueType(), LHSHi, RHSHi);
+
----------------
LHSHiVT?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:7792-7793
 
   if (VT != In2.getValueType())
     return SDValue();
 
----------------
Not new but can this be removed? as it can never happen given the `SrcVT.bitsLT/SrcVT.bitsGT` code above.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:7796
+  if (isFixedSVE) {
+    assert(isTypeLegal(VT) && "Expected only legal fixed-width types");
+    VT = getContainerForFixedLengthVector(DAG, VT);
----------------
This can be assumed, plus `getContainerForFixedLengthVector` will ensure the type is legal anyway.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:7863-7867
+  if (isFixedSVE) {
+    VT = Op.getValueType();
+    IntVT = VT.changeTypeToInteger();
+    BSP = convertFromScalableVector(DAG, IntVT, BSP);
+  }
----------------
Bookending the fixed length lowering like this has pitfalls and can complicate the code.  It's better to just rewrite the fixed length operations using scalable vector types and then let the scalable vector lowering handle any complexity.  Towards the start of the function you can do:
```
EVT ContainerVT = getContainerForFixedLengthVector(DAG, VT);
In1 = convertToScalableVector(DAG, ContainerVT, In1);
In2 = convertToScalableVector(DAG, ContainerVT, In2);
Res = getNode(ISD::FCOPYSIGN, ContainerVT , In1, In2)
return convertFromScalableVector(DAG, ContainerVT, Res);
```
This way it doesn't matter how complicated the scalable vector lowering gets.  Doing this also means you no longer need sve2-fixed-length-fcopysign.ll because there's nothing SVE2 special about the lowering code you've added (i.e. the original sve2-fcopysign.ll tests are good enough to protect that functionality).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128642