[PATCH] D107657: [RISCV][VP] Add support for VP_REDUCE_* operations

Mon Aug 9 05:14:16 PDT 2021

frasercrmck added a comment.

In D107657#2931740 <https://reviews.llvm.org/D107657#2931740>, @craig.topper wrote:

> Does TLI.expandVecReduce need any updates?

To be honest I was thinking of leaving that sort of thing until a future patch because I don't believe any of the existing VP nodes can be anything but legal or custom legalized, so it may require a wider discussion. I think the VP SDNodes should be legalizable as I don't think the `ExpandVectorPredication` pass's job is to perform vector type legalization. So the SelectionDAG should expect to encounter VP operations which should be split, widened, etc. Does @simoll have any thoughts or plans to introduce this? I don't believe the VP reference patch has support for this kind of legalization.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2055
+// Promote either the vector operand of a VECREDUCE_* or VP_REDUCE_* node, or
+// the start value of a VP_REDUCE node.
+SDValue DAGTypeLegalizer::PromoteIntOp_VEC_VP_REDUCE(SDNode *N, unsigned OpNo) {
----------------
craig.topper wrote:
> Are we allowing the start value to have a different type than the result? Or should we make them them same and handle start promotion when we handle result promotion. I think that would also mean we need to insert an appropriate extend if promoting the vector forces the result type to be enlarged?
I think it's probably best if we say that the start and result should have the same type. I can see that promoting the start value when promoting the result value is nice and consistent in that regard. Is that something that's typically done?

And yes I think you're right about also extending the start value in the case that the vector is promoted.

================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:628
+
+      setOperationAction(ISD::VP_REDUCE_FADD, VT, Custom);
+      setOperationAction(ISD::VP_REDUCE_SEQ_FADD, VT, Custom);
----------------
craig.topper wrote:
> Aren't these covered by the FloatingPointVPOps loop?
Ah yes they are, thanks; an issue with my local refactoring.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107657