[PATCH] D89162: [SVE][WIP] Lower fixed length VECREDUCE_SEQ_FADD operation

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

Are you planning to add support for the normal VECREDUCE_FADD?  I ask because that's likely to see more initial use upstream than the SEQ variant.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:1154-1158
+  case ISD::VECREDUCE_SEQ_FADD:
+  case ISD::VECREDUCE_SEQ_FMUL:
+    Action = TLI.getOperationAction(
+        Node->getOpcode(), Node->getOperand(1).getValueType());
+    break;
----------------
You'll need to move these below the main VECREDUCE_ options because VECREDUCE_FADD & VECREDUCE_FMUL only take a single operand.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1131-1134
+      for (auto VT : {MVT::v2f32, MVT::v4f32, MVT::v1f64, MVT::v2f64})
+        // FIXME: There is a NEON FADDA instruction, but it's not
+        //        currently supported.
+        setOperationAction(ISD::VECREDUCE_SEQ_FADD, VT, Custom);
----------------
I doubt it's worth custom lowering `v1f64` being the expansion will be a single neon ADD. Or is this because there is current no expand code of the SEQ reductions?

What about operations on vectors of `f16`.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16147
 
+SDValue AArch64TargetLowering::LowerFixedLengthReductionAccToSVE(
+    unsigned Opcode, SDValue ScalarOp, SelectionDAG &DAG) const {
----------------
Given there's likely to be only one use of this I think calling this `LowerVECREDUCE_SEQ_FADD` is more inline with the naming of the other lowering functions.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16158-16161
+  // FIXME: This produces a masked MOV. Do we care about the false lanes?
+  AccOp = DAG.getNode(ISD::INSERT_VECTOR_ELT, DL, ContainerVT,
+                      DAG.getUNDEF(ContainerVT), AccOp,
+                      DAG.getConstant(0, DL, MVT::i64));
----------------
Looks like we're missing some obvious patterns.  I've created D89235 to add them.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h:223-224
     case Intrinsic::vector_reduce_fadd:
+      if (ST->hasSVE())
+        return false;
+    LLVM_FALLTHROUGH;
----------------
This'll need to be more verbose because having SVE is only safe for scalable vectors.  You'll need to look at the useSVEForFixedLengthVectorVT question if II is operating on fixed length types.


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-fp-reduce.ll:57
+; VBITS_GE_256-NEXT: mov z[[ACC:[0-9]+]].s, [[PG1]]/m, z0.s
+; VBITS_GE_256-NEXT: Xfadda s0, [[PG]], s[[ACC]], [[OP]].s
+; VBITS_GE_256-NEXT: ret
----------------
Looks like a stray X here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89162