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

[Craig Topper via Phabricator via llvm-commits]

craig.topper 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
----------------
vale -> value


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:241
+
+// Specialized helper macro for reductions (%x, %mask, %evl).
+#ifdef HELPER_REGISTER_REDUCTION_VP
----------------
Is this comment still accurate with start value?


================
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
----------------
Should this be %acc or %start?


================
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)
----------------
Should this be lined up with vp_reduce_fadd on the line above?


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:376
+  case Intrinsic::vp_reduce_fadd:
+    Reduction = Builder.CreateFAddReduce(Start, RedOp);
+    break;
----------------
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);    
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104308