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
7244

Why int instead of unsigned?

7245

Capitalize

7252

reassocation->reassociation

7257

Why getNode()?

7271

Pass Opc by value not by reference

7273

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

7280

netrual -> neutral

7289

Capitalize anotherOpIdx

Use (1 - ReduceIdx) instead of !reduceIdx.

7293

auto C -> auto *C

7300

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

7302

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
7239

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

7252

disallow->disallows

7258

Extract.getOperand(0)

7262

Reduce.getOperand(2)

7273

I think you can use isNullFPConstant

8452

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
7254

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
7254

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
7254

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
7254

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
7265

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.

7273

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
7273

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
7273

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
7273

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
8452

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
8452

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
8452

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.