[PATCH] D59259: [AArch64] Use faddp to implement fadd reductions.

Fri Apr 5 08:57:19 PDT 2019

sdesmalen marked an inline comment as done.
sdesmalen added a comment.

ping!

================
Comment at: include/llvm/Target/TargetSelectionDAG.td:284
+def SDT_FPVecReduce : SDTypeProfile<1, 1, [
+  SDTCisFP<0>, SDTCisVec<1>
+]>;
----------------
sdesmalen wrote:
> RKSimon wrote:
> > What happened to the accumulator argument? IMO we shouldn't have it all but the intrinsic currently defines it.
> The accumulator argument is not part of `VECREDUCE_FADD`, and is only represented in the  `VECREDUCE_STRICT_FADD` for the reason that it _must_ start off with that value as part of the ordered reduction, where for the non-strict `VECREDUCE_FADD` it can be accumulated separately using a regular `FADD`. There should probably be a separate `SDT_FPStrictVecReduce` when we implement this for `VECREDUCE_STRICT_FADD`.
> 
> Looking in `SelectionDAGBuilder::visitVectorReduce()`  I see that the accumulator operand is actually ignored for non-strict fadd reductions, which is something that we'll need to fix.
Please note that I have pinged off the question on the accumulator argument of the LLVM IR intrinsic to the mailing list in an RFC.
For the current definition of the SDNodes, having no accumulator argument here is correct.

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

https://reviews.llvm.org/D59259