This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Fold vector binary operatrion into select with identity constant.
ClosedPublic

Authored by jacquesguan on Aug 10 2022, 1:38 AM.

Details

Summary

This patch implements shouldFoldSelectWithIdentityConstant for RISCV. It would try to generate vmerge after the binary instruction and let them folded to maksed instruction later.

Diff Detail

Event Timeline

jacquesguan created this revision.Aug 10 2022, 1:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 1:38 AM
jacquesguan requested review of this revision.Aug 10 2022, 1:38 AM

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.

Use DAG combiner instead of pattern.

remove empty line.

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.

jacquesguan retitled this revision from [RISCV] Add binary mask pattern for vector integer binary instructions. to [RISCV] Fold scalable binary integer vector op and vector select op to RVV VL Op..Aug 12 2022, 5:26 AM
jacquesguan edited the summary of this revision. (Show Details)
StephenFan added inline comments.Aug 12 2022, 9:29 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
8317 ↗(On Diff #452148)

Is it possible to use a macro definition to reduce code lines?

Address comment.

jacquesguan added inline comments.Aug 12 2022, 7:50 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
8317 ↗(On Diff #452148)

Done.

jrtc27 added inline comments.Aug 12 2022, 7:54 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
8314 ↗(On Diff #452362)

Imm isn't that clear, this is really the identity

8357 ↗(On Diff #452362)

Why the blank line when the others don't have one?

8317 ↗(On Diff #452148)

Undefine helper macros like this once you're done with them

jacquesguan added inline comments.Aug 14 2022, 6:36 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
8314 ↗(On Diff #452362)

Which word you suggest to use?

craig.topper added inline comments.Aug 14 2022, 6:48 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
981 ↗(On Diff #452362)

Why !hasStdExtZbb()?

8347 ↗(On Diff #452362)

VLMaxSentinel is not to be used in SelectionDAG anymore. Use getRegister(RISCV::X0

craig.topper added inline comments.Aug 14 2022, 6:53 PM
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.

craig.topper added inline comments.Aug 14 2022, 7:01 PM
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.

craig.topper requested changes to this revision.Aug 14 2022, 7:03 PM
This revision now requires changes to proceed.Aug 14 2022, 7:03 PM

Address comment.

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.

craig.topper added inline comments.Aug 15 2022, 1:55 PM
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.

craig.topper requested changes to this revision.Aug 15 2022, 2:00 PM
This revision now requires changes to proceed.Aug 15 2022, 2:00 PM

Address comment.

jacquesguan added inline comments.Aug 16 2022, 6:53 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
8275 ↗(On Diff #452603)

Done.

8286 ↗(On Diff #452603)

Done.

8293 ↗(On Diff #452603)

Done.

8333 ↗(On Diff #452603)

Done.

8337 ↗(On Diff #452603)

Done.

981 ↗(On Diff #452362)

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.

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.

sophia246 removed a subscriber: sophia246.

How much does this overlap with implementing RISCVTargetLowering::shouldFoldSelectWithIdentityConstant and letting the generic DAGCombiner do this?

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?

Use RISCVTargetLowering::shouldFoldSelectWithIdentityConstant to make it.

jacquesguan retitled this revision from [RISCV] Fold scalable binary integer vector op and vector select op to RVV VL Op. to [RISCV] Fold vector binary operatrion into select with identity constant..Sep 21 2022, 1:04 AM
jacquesguan edited the summary of this revision. (Show Details)

How much does this overlap with implementing RISCVTargetLowering::shouldFoldSelectWithIdentityConstant and letting the generic DAGCombiner do this?

Done.

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.

rebase and add fp test.

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.

Done, I add FP cases.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
2220 ↗(On Diff #461806)

Done.

jacquesguan edited the summary of this revision. (Show Details)Oct 21 2022, 12:20 AM
craig.topper added inline comments.Nov 16 2022, 9:22 PM
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.

Address comment.

jacquesguan added inline comments.Nov 17 2022, 3:19 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1214 ↗(On Diff #469486)

Done, thanks.

This revision is now accepted and ready to land.Dec 5 2022, 4:01 PM
This revision was landed with ongoing or failed builds.Dec 5 2022, 7:19 PM
This revision was automatically updated to reflect the committed changes.