Page MenuHomePhabricator

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

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.

I leave XOR not enable now, since it would affect one X86 test.

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
8521

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
8521

Done.

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

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

8518

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

8521

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
8518

Which word you suggest to use?

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

Why !hasStdExtZbb()?

8551

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
8494

std::swap?

8528

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

8534

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
8530

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

8478

Done.

8494

Done.

8521

Done.

8528

Done.

8530

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

8534

Done.

8551

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
990

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

8480

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.

8491

Imm -> IdentityImm

8498

Use TargetLowering::isCommutativeBinOp?

8538

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?

8542

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
990

Done.

8480

Done.

8491

Done.

8498

Done.

8538

Done.

8542

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..Wed, Sep 21, 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.