[PATCH] D104308: [VP] Add vector-predicated reduction intrinsics

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked an inline comment as done.
frasercrmck added inline comments.


================
Comment at: llvm/docs/LangRef.rst:18750
+on each enabled lane, multiplying it by the scalar ``start_value``. Disabled
+lanes are treated as containing the neutral vale ``1`` (i.e. having no
+effect on the reduction operation). If the vector length is zero, the result is
----------------
craig.topper wrote:
> vale -> value
Nice spot


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:241
+
+// Specialized helper macro for reductions (%x, %mask, %evl).
+#ifdef HELPER_REGISTER_REDUCTION_VP
----------------
craig.topper wrote:
> Is this comment still accurate with start value?
Nope, but it is now, thanks for catching that.


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:297
+
+// Specialized helper macro for reductions with a starting value (%acc, %x, %mask, %evl).
+#ifdef HELPER_REGISTER_REDUCTION_SEQ_VP
----------------
craig.topper wrote:
> Should this be %acc or %start?
Aye it should be `%start` but I've reworked the documentation anyway; this specialized macro is now really for reductions with separate "seq" forms. Cheers.


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:313
+HELPER_REGISTER_REDUCTION_SEQ_VP(vp_reduce_fadd, VP_REDUCE_FADD,
+                                   VP_REDUCE_SEQ_FADD,
+                                   experimental_vector_reduce_fadd)
----------------
craig.topper wrote:
> Should this be lined up with vp_reduce_fadd on the line above?
Done, cheers.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:376
+  case Intrinsic::vp_reduce_fadd:
+    Reduction = Builder.CreateFAddReduce(Start, RedOp);
+    break;
----------------
craig.topper wrote:
> Is the documentation for this in IRBuilder incorrect? It says the accumulator is for ordered reductions. Is it also used for unordered?
> 
> ```
>   /// Create a vector fadd reduction intrinsic of the source vector.                                                                                                                                      
>   /// The first parameter is a scalar accumulator value for ordered reductions.                                                                                                                           
>   CallInst *CreateFAddReduce(Value *Acc, Value *Src);                                                                                                                                                     
>                                                                                                                                                                                                           
>   /// Create a vector fmul reduction intrinsic of the source vector.                                                                                                                                      
>   /// The first parameter is a scalar accumulator value for ordered reductions.                                                                                                                           
>   CallInst *CreateFMulReduce(Value *Acc, Value *Src);    
> ```
Yeah so the unordered reduction intrinsics do indeed have an accumulator, so this is misleading. However, since the only difference between ordered and unordered intrinsics is the presence of the `reassoc` flag, this method always creates an ordered reduction. It's up to the user to add the flag later. That's what we do in `replaceOperation` by transferring any existing fast-math flags on to the expanded reduction.

>From the SelectionDAG's perspective the unordered reductions //don't// have an accumulator, because it's split out early in the SelectionDAGBuilder. I wonder if that's where the confusion came from.

I think I still support addressing this documentation though. I can do that in a separate patch as it's likely to spawn discussion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104308