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

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

Done.

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

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

8317

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

8357

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

jacquesguan added inline comments.Aug 14 2022, 6:36 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
8314

Which word you suggest to use?

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

Why !hasStdExtZbb()?

8347

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

std::swap?

8324

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

8330

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

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

Done.

8317

Done.

8324

Done.

8326

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

8330

Done.

8347

Done.

8357

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
981

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

8276

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.

8287

Imm -> IdentityImm

8294

Use TargetLowering::isCommutativeBinOp?

8334

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?

8338

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
981

Done.

8276

Done.

8287

Done.

8294

Done.

8334

Done.

8338

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
1184

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
1184

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.