[PATCH] D87796: [SVE][WIP] Lower fixed length VECREDUCE_ADD to Scalable

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15924
 
+SDValue AArch64TargetLowering::LowerFixedLengthReductionToSVE(unsigned Op,
+    SDValue ScalarOp, SelectionDAG &DAG) const {
----------------
Using Op here is confusing because that is normally what is used to reference the passed in SDValue.  Existing code typically uses `Opc` or `Opcode` when naming ISD node variables. 


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15928
+  SDValue VecOp = ScalarOp.getOperand(0);
+  EVT VT = VecOp.getValueType();
+
----------------
VT is normally used to mean the node's result value type.  Perhaps InVT or SrcVT?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15931
+  // UADDV always returns a vector of i64 result.
+  EVT ResVT = (Op == AArch64ISD::UADDV_PRED) ? MVT::v2i64 : VT;
+
----------------
This is the result VT of the reduction so you could use getPackedSVEVectorVT passing in the element type of SrcVT.  Something like:
```
EVT RdxVT = getPackedSVEVectorVT(Op == AArch64ISD::UADDV_PRED ? MVT::i64 : SrcVT.getVectorElementType());
```
Or you can move the code lower and use ContainerVT instead of VT. e.g.
```
EVT RdxVT = (Op == AArch64ISD::UADDV_PRED) ? MVT::nxv2i64 : ContainerVT;
```




================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15939-15940
+  VecOp = convertToScalableVector(DAG, ContainerVT, VecOp);
+  SDValue Rdx = DAG.getNode(Op, DL, ResContainerVT, Pg, VecOp);
+  SDValue Res = convertFromScalableVector(DAG, ResVT, Rdx);
+
----------------
I don't think you need convertFromScalableVector here as you can just extract the scalar result directly from the scalable vector result of the reduction.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15950-15951
+  // VEC_REDUCE nodes expect an element size result.
+  // FIXME: This should be a no-op trunc on the other nodes. Confirm
+  //        and remove this comment.
+  return DAG.getNode(ISD::TRUNCATE, DL, ScalarOp.getValueType(), Ext);
----------------
It would be better to protect the truncate with something like `Rdx.ElementType != Op.getValueType()`. I say this because I don't currently see a reason why this function cannot be used for floating-point reductions other than this unprotected truncate.


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-int-reduce.ll:47
+; CHECK-LABEL: uaddv_v32i8:
+; VBITS_GE_256: ptrue [[PG:p[0-9]+]].b, vl[[#min(VBYTES,32)]]
+; VBITS_GE_256-DAG: ld1b { [[OP:z[0-9]+]].b }, [[PG]]/z, [x0]
----------------
Is `#min()` required here? Same goes for the other tests.


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

https://reviews.llvm.org/D87796