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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked 3 inline comments as done.
frasercrmck added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2115
+SDValue DAGTypeLegalizer::PromoteIntOp_VP_REDUCE(SDNode *N, unsigned OpNo) {
+  SDLoc DL(N);
+  SDValue Op = PromoteIntOpVectorReduction(N, N->getOperand(OpNo));
----------------
frasercrmck wrote:
> craig.topper wrote:
> > What values of OpNo do we expect here? 0 shouldn't happen since it was handled in result promotion. 1 clearly will happen. I'm not sure about 2 and 3, but using PromoteIntOpVectorReduction for them would be incorrect.
> Yeah you're right, thanks. I think 0 is out of the runnings after the latest round of changes. I don't think we can get 3 because EVL is promoted early to a target-specific type which is assumed/required to be legal.
> 
> So yeah, looking at the handling of `MGATHER` I think I need to update this function to handle the mask at `OpNo` 2. We won't have tests for it but I can maybe manufacture something locally to make sure it's not obviously bad.
Done so only operands 2 and 1 can be promoted.


================
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:
> frasercrmck wrote:
> > 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.
> > 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?
> 
> Yes. If you do them at the same time then there is no transient state where they have a different type. We could even assert it in getNode we want.
> 
> > 
> > And yes I think you're right about also extending the start value in the case that the vector is promoted.
> 
> 
Cheers for the advice. I've done that though I didn't add the assert. Do you think that's worth adding as part of this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107657