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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/docs/LangRef.rst:18582
+
+The first operand must be a vector of integer values. The result type must be
+the element type of the first operand. The second operand is the vector mask
----------------
craig.topper wrote:
> With the start value added, I think it's the second operand now?
Yes, thanks. I didn't do a proper sweep over the docs. I've made further changes to them so it might be worth going over them again.


================
Comment at: llvm/docs/LangRef.rst:18807
+on each enabled lane. Disabled lanes are treated as containing the neutral
+value (``1``) (i.e. having no effect on the reduction operation). Thus if the
+vector length is zero, the result is the start value.
----------------
craig.topper wrote:
> Neutral value is -1 or UINT_MAX for AND
Good spot, thanks. Updated the "equivalent to" section below, too.


================
Comment at: llvm/docs/LangRef.rst:19175
+The neutral value is dependent on the :ref:`fast-math flags <fastmath>`. If no
+flags are set, the neutral value is ``-QNAN``. If ``nnan``  and ``ninf`` are
+both set, then the neutral value is the smallest floating-point value for the
----------------
craig.topper wrote:
> Does -QNAN mean anything? Isn't the sign of a NaN mostly ignored.
I have heard that the sign of NaNs is often ignored but I'm not sure it's guaranteed to be ignored? `-QNAN` does have a bit representation and `ConstantFP` and `APFloat` both take a `Negative` parameter to their `getQNAN` methods. It's also what `SelectionDAG::getNeutralElement` is doing for `FMAXNUM`.


================
Comment at: llvm/docs/LangRef.rst:19182
+the '``llvm.maxnum.*``' intrinsic). That is, the result will always be a number
+unless all elements of the vector are ``NaN``. Note that this means if all
+lanes are disabled the result will *not* be a number. For a vector with maximum
----------------
craig.topper wrote:
> Even if all vector elements are NaN, the result would depend on the start value
Yes true. I've replaced that sentence now.


================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:456
+
+  unsigned getStartParamPos() const;
+  unsigned getVectorParamPos() const;
----------------
Not sure whether to keep this in or not, now that all vp reductions have start parameter? Currently start is always 0 and vector is always 1.


================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:465
+  static bool classof(const IntrinsicInst *I) {
+    return VPIntrinsic::isVPIntrinsic(I->getIntrinsicID());
+  }
----------------
craig.topper wrote:
> Should this be isVPReduction?
Oh good spot, thanks. I've fixed that and added a unit test for these `VPReductionIntrinsic` methods.


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:251
+
+// llvm.vp.reduce.add(accu,x,mask,vlen)
+HELPER_REGISTER_REDUCTION_VP(vp_reduce_add, VP_REDUCE_ADD,
----------------
craig.topper wrote:
> Why accu instead of start?
I think the reference patch uses/used `accu` so I was flip-flopping between the two. I agree that "start" is better though.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:264
+    llvm_unreachable("Expecting a VP reduction intrinsic");
+  case Intrinsic::vp_reduce_add:
+  case Intrinsic::vp_reduce_or:
----------------
craig.topper wrote:
> I was wondering if we could go through ConstantExpr::getBinOpIdentity to avoid repeating the constants in multiple places but we'd still need special handling for min/max
Coming at it from a slightly different angle, I was wondering if there should be a single source of truth for neutral reduction elements between IR and SelectionDAG.


================
Comment at: llvm/test/Verifier/vp-intrinsics.ll:34
+define void @test_vp_reduction(<8 x i32> %vi, <8 x float> %vf, float %f, <8 x i1> %m, i32 %n) {
+  %r0 = call i32 @llvm.vp.reduce.add.v8i32(<8 x i32> %vi, <8 x i1> %m, i32 %n)
+  %r1 = call i32 @llvm.vp.reduce.mul.v8i32(<8 x i32> %vi, <8 x i1> %m, i32 %n)
----------------
craig.topper wrote:
> Start value is missing
Cheers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104308