This patch implements shouldFoldSelectWithIdentityConstant for RISCV. It would try to generate vmerge after the binary instruction and let them folded to maksed instruction later.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
We've been discussing doing this as a post isel peephole or a DAG combine in D130442. That should have much less impact on the size of the generated isel table.
It might not be a good idea to do this fold without checking that the select is the only user of binary op. If it's not the only user you'll duplicate the binary op and there will be a masked and unmasked version. This can increase register pressure and use more execution resources.
Thanks for your advice, I swtich to the DAG combiner. And I do not support fixed vector in this revision as I have not finished its test cases. I will add anothe revision to make it.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
8317 ↗ | (On Diff #452148) | Is it possible to use a macro definition to reduce code lines? |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
8317 ↗ | (On Diff #452148) | Done. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
8314 ↗ | (On Diff #452362) | Which word you suggest to use? |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
8290 ↗ | (On Diff #452362) | std::swap? |
8324 ↗ | (On Diff #452362) | Some of these operations aren't commutable. If the select is on the LHS of the SUB this transform isn't valid. |
8330 ↗ | (On Diff #452362) | 1 is not an identity value for urem or srem, it would always return 0. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
8326 ↗ | (On Diff #452362) | 1 is also not the identify value for MULHS/MULHU. This returns the high half of the product. MULHU(a, 1) produces 0 since the high product is 0. MULHS(a, 1) is a vector filled with the sigh bit of a. |
Remove blank line.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
981 ↗ | (On Diff #452362) | if (Subtarget.hasStdExtZbb()) setTargetDAGCombine({ISD::UMAX, ISD::UMIN, ISD::SMAX, ISD::SMIN}); Because if we have Zbb, we already set ISD::UMIN and ISD::UMAX. |
8290 ↗ | (On Diff #452362) | Done. |
8324 ↗ | (On Diff #452362) | Done. |
8326 ↗ | (On Diff #452362) | Done, I removed MULHS, MULHU, UREM and SREM. |
8330 ↗ | (On Diff #452362) | Done. |
8347 ↗ | (On Diff #452362) | Done. |
8357 ↗ | (On Diff #452362) | Done. |
8317 ↗ | (On Diff #452148) | Done. |
Can you land the tests and rebase this change please? Be helpful to see the deltas.
Please also add tests (for only one op flavor is fine) for binop with multiple uses, and other negative edge cases.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
8275 ↗ | (On Diff #452603) | This isn't protected against illegal types. DAGCombiners can see any types. My suggestion is to defer this combine until DCI.isAfterLegalizeDAG() which ensure types are legalized and that we don't create _VL operations too early. |
8286 ↗ | (On Diff #452603) | Imm -> IdentityImm |
8293 ↗ | (On Diff #452603) | Use TargetLowering::isCommutativeBinOp? |
8333 ↗ | (On Diff #452603) | Technically you need to truncate the splat_vector input to element size. But maybe all your tests are before type legalization so you don't test any SPLAT_VECTORS where the scalar input is larger than the element type? |
8337 ↗ | (On Diff #452603) | VMV_V_X_VL is allowed to have scalar values both larger and smaller than the element type. |
981 ↗ | (On Diff #452362) | It's harmless to do it again and would make the code less confusing. |
Done, I add a pre-commit test in https://reviews.llvm.org/D131950, and the negative test is added in the llvm/test/CodeGen/RISCV/rvv/vadd-sdnode.ll.
How much does this overlap with implementing RISCVTargetLowering::shouldFoldSelectWithIdentityConstant and letting the generic DAGCombiner do this?
Thanks, it should work, but I need https://reviews.llvm.org/D132923 to fold the transformed DAG, could you mind to have a review?
foldSelectWithIdentityConstant handles FP as well, but you have no FP tests. It doesn't look like you did anything to disable FP unless I missed it.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
2220 ↗ | (On Diff #461806) | This needs to be rebased. |
Done, I add FP cases.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
2220 ↗ | (On Diff #461806) | Done. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
1214 ↗ | (On Diff #469486) | This is imprecise for FP which has a different ELEN for Zve64f and Zve32x. I think you can ignore it for scalable vectors since those elements always have to be supported if we see them. For fixed vectors we should check isTypeLegal. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
1214 ↗ | (On Diff #469486) | Done, thanks. |