[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: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
----------------
With the start value added, I think it's the second operand now?


================
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.
----------------
Neutral value is -1 or UINT_MAX for AND


================
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
----------------
Does -QNAN mean anything? Isn't the sign of a NaN mostly ignored.


================
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
----------------
Even if all vector elements are NaN, the result would depend on the start value


================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:465
+  static bool classof(const IntrinsicInst *I) {
+    return VPIntrinsic::isVPIntrinsic(I->getIntrinsicID());
+  }
----------------
Should this be isVPReduction?


================
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,
----------------
Why accu instead of start?


================
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:
----------------
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


================
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)
----------------
Start value is missing


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104308