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
8107

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
8107

Done.

jrtc27 added inline comments.Aug 12 2022, 7:54 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
8064

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

8104

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

8107

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
8104

Which word you suggest to use?

craig.topper added inline comments.Aug 14 2022, 6:48 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
994

Why !hasStdExtZbb()?

8137

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
8080

std::swap?

8114

Some of these operations aren't commutable. If the select is on the LHS of the SUB this transform isn't valid.

8120

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
8116

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
994
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.

8064

Done.

8080

Done.

8107

Done.

8114

Done.

8116

Done, I removed MULHS, MULHU, UREM and SREM.

8120

Done.

8137

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
994

It's harmless to do it again and would make the code less confusing.

8066

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.

8077

Imm -> IdentityImm

8084

Use TargetLowering::isCommutativeBinOp?

8124

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?

8128

VMV_V_X_VL is allowed to have scalar values both larger and smaller than the element type.

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
994

Done.

8066

Done.

8077

Done.

8084

Done.

8124

Done.

8128

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

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

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.