[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/lib/CodeGen/ExpandVectorPredication.cpp:376
+  case Intrinsic::vp_reduce_fadd:
+    Reduction = Builder.CreateFAddReduce(Start, RedOp);
+    break;
----------------
frasercrmck wrote:
> 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.
I tried to address this in D107753.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104308