Transform (<bop> x, (reduce.<bop> vec, splat(neutral_element))) to
(reduce.<bop> vec, splat (x)).
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
7434 | Why int instead of unsigned? | |
7435 | Capitalize | |
7442 | reassocation->reassociation | |
7447 | Why getNode()? | |
7461 | Pass Opc by value not by reference | |
7463 | auto C -> auto *C We always want to be able to see if something is a pointer. | |
7470 | netrual -> neutral | |
7479 | Capitalize anotherOpIdx Use (1 - ReduceIdx) instead of !reduceIdx. | |
7483 | auto C -> auto *C | |
7490 | Use ScalarV.getOperand(0) instead of ScalarV->getOperand(0) | |
7492 | Don't use UpdateNodeOperands it scares me because it can trigger surprising DAG changes that can invalidate the SDValues you're holding.. Use getNode to create new nodes. |
Fix those issues Craig raised. And I also removed anotherOpIdx since I think (1 - ReduceIdx) is enough clear.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
7429 | I think you also need isNullConstant(V.getOperand(1) to make sure we're extracting the lowest element. | |
7442 | disallow->disallows | |
7448 | Extract.getOperand(0) | |
7452 | Reduce.getOperand(2) | |
7463 | I think you can use isNullFPConstant | |
8632 | Can we just lump all these together calling combineBinOpToReduce directly until we need specific combines for them? |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
7444 | The ReduceIdx is relevant here. The order of the fadd operands doesn't matter. The reduce operation is before the fadd, we need to know if the fadd can be moved before the reduce. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
7444 | VECREDUCE_FADD_VL always has reassociation, so I think I only need to check N->getFlags().hasAllowReassociation() to make sure fadd can be moved before the reduce. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
7444 | I meant to say “ReduceIdx ISN’T relevant” |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
7444 | Yeah. I understand I should remove the condition. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
7455 | I missed it before, but I believe you need to be able to prove that the VL parameter for the splat isn't 0. In the usual case it's a constant 1 right? S o you can probably just check that it's a constant that isn't 0. | |
7463 | I think you want the isNullFPConstant in the if and a return true here. Just because hasNoSignedZeros is true doesn't mean that -0.0 can't appear. |
Taking a step back a bit, this is also a target-independent combine we'd want to perform on vp.reduce operations, right? Is it possible that we may want to lower/combine vector.reduce to vp.reduce and let this sort of combine happen in a target-independent combine?
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
7463 | When hasNoSignedZeros enabling, compiler insert +0.0 into VECREDUCE_FADD_VL. I think the only way to replace the start value is this combiner combineBinOpToReduce, so the start value is -0.0 is caused by FADD with -0.0. But I think DAGCombiner could fix it. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
7463 | If we use vp.reduce.fadd.nxv2f16 the compiler doesn't insert +0.0 if the user provided -0.0. declare half @llvm.vp.reduce.fadd.nxv2f16(half, <vscale x 2 x half>, <vscale x 2 x i1>, i32) define half @vpreduce_fadd_nxv2f16(half %s, <vscale x 2 x half> %v, <vscale x 2 x i1> %m, i32 zeroext %evl) { %r = call reassoc half @llvm.vp.reduce.fadd.nxv2f16(half -0.0, <vscale x 2 x half> %v, <vscale x 2 x i1> %m, i32 %evl) %t = fadd reassoc half %r, %s ret half %t } |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
7463 | Thank you. I understand it. |
I think that makes sense, but I have a few questions.
-Do we currently mark VP_REDUCE nodes as Expand on targets that don't support it? I don't see anything in TargetLoweringBase::initActions, but maybe I missed it. I think we would need that fixed to know if we could do the combine so that we only do it on targets that support it.
-What would we use for VL for the VP_REDUCE from a generic combine? vscale * known minimum element count? Then we'd need to detect that and replace it with RISCV::X0 for RISCV?
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
8632 | This wasn't addressed. I don't think we want individual performUMAXCombine, performUMINCombine, etc. functions. |
VP_REDUCE nodes are marked as Custom. VP_REDUCE opcodes are elements of IntegerVPOps and FloatingPointVPOps whose elements would be marked as Custom.
-What would we use for VL for the VP_REDUCE from a generic combine? vscale * known minimum element count? Then we'd need to detect that and replace it with RISCV::X0 for RISCV?
I have another question. If we want to transform VECREDUCE to VP_REDUCE before legalization, what is the VL of vectors needed split like <vscale x 16 x i64>?
Since we need to transform VECREDUCE to VP_REDUCE before legalization for Fraser's method, we need to deal with illegal type of VECREDUCE. If we don't have idea to deal with illegal type nodes of VECREDUCE, how about we just use my original combine?
They aren't marked Custom or Expand on other targets like X86. They are marked Legal because that is the default. So we can't write a generic DAGCombine until we fix every other target to mark them as Expand. Otherwise the DAGCombine will start creating VP_REDUCE on other targets that don't really support it.
-What would we use for VL for the VP_REDUCE from a generic combine? vscale * known minimum element count? Then we'd need to detect that and replace it with RISCV::X0 for RISCV?
I have another question. If we want to transform VECREDUCE to VP_REDUCE before legalization, what is the VL of vectors needed split like <vscale x 16 x i64>?
The VL would be 16 * ISD::VSCALE. Then we need more DAGCombine's to make sure that gets split nicely into 8*VSCALE after the split. Then we need to teach RISC-V lowering to detect VSCALE based VL to map to X0 for vsetvli.
This all seems like more work than this patch right now.
I'm fine with this patch. It makes the most sense with the way things currently are.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
8632 | Please address this comment and I will approve this patch. |
Yeah, this patch is good as it is Thanks for going along with my suggestion and finding out what still needs to be done! Sorry for not getting back to you promptly.
As Craig says, LGTM with his request.
The update replaces specefic function only calling combineBinOpToReduce with combineBinOpToReduce and uses array input version setTargetDAGCombine.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
8632 | I had lumped them together and call combineBinOpToReduce directly in the latest update. |
I think you also need isNullConstant(V.getOperand(1) to make sure we're extracting the lowest element.