This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add DAGCombine to fold base operation and reduction.
ClosedPublic

Authored by fakepaper56 on Mar 28 2022, 1:50 AM.

Details

Summary

Transform (<bop> x, (reduce.<bop> vec, splat(neutral_element))) to
(reduce.<bop> vec, splat (x)).

Diff Detail

Event Timeline

fakepaper56 created this revision.Mar 28 2022, 1:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2022, 1:50 AM
fakepaper56 requested review of this revision.Mar 28 2022, 1:50 AM
craig.topper added inline comments.Mar 28 2022, 10:21 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7434

Why int instead of unsigned?

7435

Capitalize

7442

reassocation->reassociation

7447

Why getNode()?

7461

Pass Opc by value not by reference

7463

auto C -> auto *C We always want to be able to see if something is a pointer.

7470

netrual -> neutral

7479

Capitalize anotherOpIdx

Use (1 - ReduceIdx) instead of !reduceIdx.

7483

auto C -> auto *C

7490

Use ScalarV.getOperand(0) instead of ScalarV->getOperand(0)

7492

Don't use UpdateNodeOperands it scares me because it can trigger surprising DAG changes that can invalidate the SDValues you're holding.. Use getNode to create new nodes.

Fix those issues Craig raised. And I also removed anotherOpIdx since I think (1 - ReduceIdx) is enough clear.

fakepaper56 marked 5 inline comments as done.

Use auto *C for C is pointer

fakepaper56 marked 6 inline comments as done.Mar 28 2022, 11:24 PM
craig.topper added inline comments.Mar 29 2022, 12:42 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7429

I think you also need isNullConstant(V.getOperand(1) to make sure we're extracting the lowest element.

7442

disallow->disallows

7448

Extract.getOperand(0)

7452

Reduce.getOperand(2)

7463

I think you can use isNullFPConstant

8632

Can we just lump all these together calling combineBinOpToReduce directly until we need specific combines for them?

craig.topper added inline comments.Mar 29 2022, 12:45 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7444

The ReduceIdx is relevant here. The order of the fadd operands doesn't matter. The reduce operation is before the fadd, we need to know if the fadd can be moved before the reduce.

fakepaper56 added inline comments.Mar 29 2022, 8:50 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7444

VECREDUCE_FADD_VL always has reassociation, so I think I only need to check N->getFlags().hasAllowReassociation() to make sure fadd can be moved before the reduce.

craig.topper added inline comments.Mar 29 2022, 8:55 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7444

I meant to say “ReduceIdx ISN’T relevant”

fakepaper56 marked an inline comment as not done.Mar 29 2022, 9:02 PM
fakepaper56 added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7444

Yeah. I understand I should remove the condition.

Fix the issues Craig raised.

disallow -> disallows

fakepaper56 marked 5 inline comments as done.Mar 29 2022, 10:15 PM

Update missing issues needed to solve.

fakepaper56 marked an inline comment as done.Mar 29 2022, 10:18 PM
craig.topper added inline comments.Mar 30 2022, 12:11 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7455

I missed it before, but I believe you need to be able to prove that the VL parameter for the splat isn't 0. In the usual case it's a constant 1 right? S o you can probably just check that it's a constant that isn't 0.

7463

I think you want the isNullFPConstant in the if and a return true here. Just because hasNoSignedZeros is true doesn't mean that -0.0 can't appear.

Taking a step back a bit, this is also a target-independent combine we'd want to perform on vp.reduce operations, right? Is it possible that we may want to lower/combine vector.reduce to vp.reduce and let this sort of combine happen in a target-independent combine?

fakepaper56 added inline comments.Mar 30 2022, 1:49 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7463

When hasNoSignedZeros enabling, compiler insert +0.0 into VECREDUCE_FADD_VL. I think the only way to replace the start value is this combiner combineBinOpToReduce, so the start value is -0.0 is caused by FADD with -0.0. But I think DAGCombiner could fix it.

fakepaper56 added a comment.EditedMar 30 2022, 2:38 AM

Taking a step back a bit, this is also a target-independent combine we'd want to perform on vp.reduce operations, right? Is it possible that we may want to lower/combine vector.reduce to vp.reduce and let this sort of combine happen in a target-independent combine?

I think it may be a good idea to do the combine when before-legalized DAG.

craig.topper added inline comments.Mar 30 2022, 3:28 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7463

If we use vp.reduce.fadd.nxv2f16 the compiler doesn't insert +0.0 if the user provided -0.0.

declare half @llvm.vp.reduce.fadd.nxv2f16(half, <vscale x 2 x half>, <vscale x 2 x i1>, i32)
                                                                                 
define half @vpreduce_fadd_nxv2f16(half %s, <vscale x 2 x half> %v, <vscale x 2 x i1> %m, i32 zeroext %evl) {
  %r = call reassoc half @llvm.vp.reduce.fadd.nxv2f16(half -0.0, <vscale x 2 x half> %v, <vscale x 2 x i1> %m, i32 %evl)
  %t = fadd reassoc half %r, %s                                                  
  ret half %t                                                                    
}
fakepaper56 added inline comments.Mar 30 2022, 7:27 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7463

Thank you. I understand it.

Fix the issue raised by reviewers.

fakepaper56 marked 3 inline comments as done.Mar 30 2022, 10:30 PM

Taking a step back a bit, this is also a target-independent combine we'd want to perform on vp.reduce operations, right? Is it possible that we may want to lower/combine vector.reduce to vp.reduce and let this sort of combine happen in a target-independent combine?

I think that makes sense, but I have a few questions.

-Do we currently mark VP_REDUCE nodes as Expand on targets that don't support it? I don't see anything in TargetLoweringBase::initActions, but maybe I missed it. I think we would need that fixed to know if we could do the combine so that we only do it on targets that support it.
-What would we use for VL for the VP_REDUCE from a generic combine? vscale * known minimum element count? Then we'd need to detect that and replace it with RISCV::X0 for RISCV?

craig.topper added inline comments.Apr 7 2022, 9:55 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
8632

This wasn't addressed. I don't think we want individual performUMAXCombine, performUMINCombine, etc. functions.

fakepaper56 added a comment.EditedApr 14 2022, 2:57 AM

-Do we currently mark VP_REDUCE nodes as Expand on targets that don't support it? I don't see anything in TargetLoweringBase::initActions, but maybe I missed it. I think we would need that fixed to know if we could do the combine so that we only do it on targets that support it.

VP_REDUCE nodes are marked as Custom. VP_REDUCE opcodes are elements of IntegerVPOps and FloatingPointVPOps whose elements would be marked as Custom.

-What would we use for VL for the VP_REDUCE from a generic combine? vscale * known minimum element count? Then we'd need to detect that and replace it with RISCV::X0 for RISCV?

I have another question. If we want to transform VECREDUCE to VP_REDUCE before legalization, what is the VL of vectors needed split like <vscale x 16 x i64>?

Since we need to transform VECREDUCE to VP_REDUCE before legalization for Fraser's method, we need to deal with illegal type of VECREDUCE. If we don't have idea to deal with illegal type nodes of VECREDUCE, how about we just use my original combine?

-Do we currently mark VP_REDUCE nodes as Expand on targets that don't support it? I don't see anything in TargetLoweringBase::initActions, but maybe I missed it. I think we would need that fixed to know if we could do the combine so that we only do it on targets that support it.

VP_REDUCE nodes are marked as Custom. VP_REDUCE opcodes are elements of IntegerVPOps and FloatingPointVPOps whose elements would be marked as Custom.

They aren't marked Custom or Expand on other targets like X86. They are marked Legal because that is the default. So we can't write a generic DAGCombine until we fix every other target to mark them as Expand. Otherwise the DAGCombine will start creating VP_REDUCE on other targets that don't really support it.

-What would we use for VL for the VP_REDUCE from a generic combine? vscale * known minimum element count? Then we'd need to detect that and replace it with RISCV::X0 for RISCV?

I have another question. If we want to transform VECREDUCE to VP_REDUCE before legalization, what is the VL of vectors needed split like <vscale x 16 x i64>?

The VL would be 16 * ISD::VSCALE. Then we need more DAGCombine's to make sure that gets split nicely into 8*VSCALE after the split. Then we need to teach RISC-V lowering to detect VSCALE based VL to map to X0 for vsetvli.

This all seems like more work than this patch right now.

Since we need to transform VECREDUCE to VP_REDUCE before legalization for Fraser's method, we need to deal with illegal type of VECREDUCE. If we don't have idea to deal with illegal type nodes of VECREDUCE, how about we just use my original combine?

I'm fine with this patch. It makes the most sense with the way things currently are.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
8632

Please address this comment and I will approve this patch.

Since we need to transform VECREDUCE to VP_REDUCE before legalization for Fraser's method, we need to deal with illegal type of VECREDUCE. If we don't have idea to deal with illegal type nodes of VECREDUCE, how about we just use my original combine?

Yeah, this patch is good as it is Thanks for going along with my suggestion and finding out what still needs to be done! Sorry for not getting back to you promptly.

As Craig says, LGTM with his request.

fakepaper56 updated this revision to Diff 424778.EditedApr 24 2022, 3:54 AM

The update replaces specefic function only calling combineBinOpToReduce with combineBinOpToReduce and uses array input version setTargetDAGCombine.

fakepaper56 marked 3 inline comments as done.Apr 24 2022, 3:59 AM
reames added a subscriber: reames.Apr 25 2022, 9:27 AM
fakepaper56 added inline comments.Apr 29 2022, 1:02 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
8632

I had lumped them together and call combineBinOpToReduce directly in the latest update.

This revision is now accepted and ready to land.Apr 29 2022, 8:30 AM
This revision was landed with ongoing or failed builds.Apr 29 2022, 11:07 PM
This revision was automatically updated to reflect the committed changes.