Page MenuHomePhabricator

[RISCV][VP] Add support for VP_REDUCE_* operations
ClosedPublic

Authored by frasercrmck on Aug 6 2021, 9:48 AM.

Details

Summary

This patch adds codegen support for lowering the vector-predicated
reduction intrinsics to RVV instructions. The process is similar to that
of the other reduction intrinsics, save for the fact that every VP
reduction has a start value. We reuse the existing custom "VL" nodes,
adding extra patterns where required to handle non-true masks.

To support these nodes, the RISCVISD::VECREDUCE_*_VL nodes have been
given an explicit "merge" operand. This is to faciliate the VP
reductions, where we must be careful to ensure that even if no operation
is performed (when VL=0) we still produce the start value. The RVV
reductions don't update the destination register under these conditions,
so we tie the splatted start value to the output register.

Diff Detail

Event Timeline

frasercrmck created this revision.Aug 6 2021, 9:48 AM
frasercrmck requested review of this revision.Aug 6 2021, 9:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2021, 9:48 AM

Does TLI.expandVecReduce need any updates?

llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
2100

Are we allowing the start value to have a different type than the result? Or should we make them them same and handle start promotion when we handle result promotion. I think that would also mean we need to insert an appropriate extend if promoting the vector forces the result type to be enlarged?

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

Aren't these covered by the FloatingPointVPOps loop?

Does TLI.expandVecReduce need any updates?

To be honest I was thinking of leaving that sort of thing until a future patch because I don't believe any of the existing VP nodes can be anything but legal or custom legalized, so it may require a wider discussion. I think the VP SDNodes should be legalizable as I don't think the ExpandVectorPredication pass's job is to perform vector type legalization. So the SelectionDAG should expect to encounter VP operations which should be split, widened, etc. Does @simoll have any thoughts or plans to introduce this? I don't believe the VP reference patch has support for this kind of legalization.

llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
2100

I think it's probably best if we say that the start and result should have the same type. I can see that promoting the start value when promoting the result value is nice and consistent in that regard. Is that something that's typically done?

And yes I think you're right about also extending the start value in the case that the vector is promoted.

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

Ah yes they are, thanks; an issue with my local refactoring.

  • remove duplicate custom lowering action for FP VP reductions
frasercrmck marked an inline comment as done.Aug 9 2021, 5:15 AM
craig.topper added inline comments.Aug 9 2021, 9:12 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
2100

I think it's probably best if we say that the start and result should have the same type. I can see that promoting the start value when promoting the result value is nice and consistent in that regard. Is that something that's typically done?

Yes. If you do them at the same time then there is no transient state where they have a different type. We could even assert it in getNode we want.

And yes I think you're right about also extending the start value in the case that the vector is promoted.

  • promote the start value in kind when promoting the vector operand forces promotion of the result type
craig.topper added inline comments.Aug 10 2021, 8:44 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
2158

What values of OpNo do we expect here? 0 shouldn't happen since it was handled in result promotion. 1 clearly will happen. I'm not sure about 2 and 3, but using PromoteIntOpVectorReduction for them would be incorrect.

frasercrmck added inline comments.Aug 10 2021, 9:25 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
2158

Yeah you're right, thanks. I think 0 is out of the runnings after the latest round of changes. I don't think we can get 3 because EVL is promoted early to a target-specific type which is assumed/required to be legal.

So yeah, looking at the handling of MGATHER I think I need to update this function to handle the mask at OpNo 2. We won't have tests for it but I can maybe manufacture something locally to make sure it's not obviously bad.

  • add promotion of mask
  • assert that we're either promoting the mask or the vector
frasercrmck marked 3 inline comments as done.Aug 31 2021, 5:06 AM
frasercrmck added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
2100

Cheers for the advice. I've done that though I didn't add the assert. Do you think that's worth adding as part of this patch?

2158

Done so only operands 2 and 1 can be promoted.

frasercrmck marked an inline comment as done.
  • rebase and make clang-format happy
craig.topper added inline comments.Sep 21 2021, 9:23 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4154

If you're going to initialize this, which I'm not sure you need to, use ISD::DELETED_NODE.

4198

(0 != 0) -> 0 surely

Also the parentheses are inconsistent between (0 == 0 -> 1) and (0 != 0) -> 1

llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td
596

Use (vti.Mask V0)

602

Use (vti.Mask V0) in place of VMV0:$vm

craig.topper added inline comments.Sep 21 2021, 10:54 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
442–443

I just pushed a change to add 'const' to these arrays so this will need to be rebased. Sorry.

  • rebase
  • address review comments
frasercrmck marked 5 inline comments as done.Sep 22 2021, 2:29 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
442–443

No worries, thanks for the heads up.

4154

Yeah I don't think I need to initialize this; CC isn't, after all. I'll keep ISD::DELETED_NODE in mind though.

4198

After some extensive verification -- not to mention soul searching -- yes, 0 != 0 is indeed 0. Luckily it's just a typo and we do indeed want to see 0 as it's the neutral value for OR/XOR. Thanks for catching my silly mistake.

I've made the parens consistent too. (0 == 0) -> 1 looked better to me at the time of making the change.

llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td
602

Ah yes good spot, thanks.

This revision is now accepted and ready to land.Sep 22 2021, 9:30 AM
This revision was landed with ongoing or failed builds.Sep 23 2021, 3:21 AM
This revision was automatically updated to reflect the committed changes.
frasercrmck marked 4 inline comments as done.