This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] (1/2) Add the tail policy argument to builtins/intrinsics.
ClosedPublic

Authored by HsiangKai on Jun 29 2021, 2:44 AM.

Details

Summary

Add the tail policy argument to Clang builtins and IR intrinsics. There are two policies for tail elements. Tail agnostic means users do not care about the values in the tail elements and tail undisturbed means the values in the tail elements need to be kept after the operation. In order to let users control the tail policy, we add an additional argument at the end of the argument list.

For unmasked operations, we have no maskedoff and the tail policy is always tail agnostic. If users want to keep tail elements under unmasked operations, they could use all one mask in the masked operations to do it. So, we only add the additional argument for masked operations for most cases. There are exceptions listed below.

In this patch, we do not handle the following cases to reduce the complexity of the patch. There could be two separate patches for them.

  • Use dest argument to control tail policy

vmerge.vvm/vmerge.vxm/vmerge.vim (add _t builtins with additional dest argument)
vfmerge.vfm (add _t builtins with additional dest argument)
vmv.v.v (add _t builtins with additional dest argument)
vmv.v.x (add _t builtins with additional dest argument)
vmv.v.i (add _t builtins with additional dest argument)
vfmv.v.f (add _t builtins with additional dest argument)
vadc.vvm/vadc.vxm/vadc.vim (add _t builtins with additional dest argument)
vsbc.vvm/vsbc.vxm (add _t builtins with additional dest argument)

  • Always has tail argument for masked/unmasked intrinsics

Vector Single-Width Integer Multiply-Add Instructions (add _t and _mt builtins)
Vector Widening Integer Multiply-Add Instructions (add _t and _mt builtins)
Vector Single-Width Floating-Point Fused Multiply-Add Instructions (add _t and _mt builtins)
Vector Widening Floating-Point Fused Multiply-Add Instructions (add _t and _mt builtins)
Vector Reduction Operations (add _t and _mt builtins)
Vector Slideup Instructions (add _t and _mt builtins)
Vector Slidedown Instructions (add _t and _mt builtins)

Discussion: https://github.com/riscv/rvv-intrinsic-doc/pull/101

Diff Detail

Event Timeline

HsiangKai created this revision.Jun 29 2021, 2:44 AM
HsiangKai requested review of this revision.Jun 29 2021, 2:44 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 29 2021, 2:44 AM
HsiangKai edited the summary of this revision. (Show Details)Jun 29 2021, 2:54 AM
khchen added a subscriber: khchen.Jun 29 2021, 6:59 AM
khchen added inline comments.
clang/utils/TableGen/RISCVVEmitter.cpp
1148

maybe the policy argument should be a constant value ("Kz")?

craig.topper added inline comments.Jun 29 2021, 9:33 AM
clang/utils/TableGen/RISCVVEmitter.cpp
1148

Agreed.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
80

This should be HasPolicyOpShift and HasPolicyOpMask to match SEWOp/VLOp naming.

138

hasPolicyOp

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
401

Probably need to mask this to bit 0. TailAgnostic = Op.getImm() & 1. As written we'll set TailAgnostic if any bit in the immediate is non-zero.

llvm/lib/Target/RISCV/RISCVInstrFormats.td
182

HasPolicyOp

HsiangKai updated this revision to Diff 355420.Jun 29 2021, 6:39 PM
  • Use constant value for tail policy argument.
  • Rename HasPolicy to HasPolicyOp.
HsiangKai updated this revision to Diff 358205.Jul 13 2021, 2:31 AM

Add the TA argument to most of the intrinsics with mask.

HsiangKai edited the summary of this revision. (Show Details)Jul 13 2021, 2:32 AM
HsiangKai updated this revision to Diff 358445.Jul 13 2021, 3:23 PM

Does lowerRISCVVMachineInstrToMCInst in RISCVMCInstLower.cpp need to know to skip the policy op?

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
405–409

Why doesn't this need to be updated?

craig.topper added inline comments.Jul 21 2021, 5:13 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td
25

Why are these the opposite polarity of what's in C?

HsiangKai updated this revision to Diff 361869.Jul 26 2021, 5:53 PM

Consider the policy operand in lowerRISCVVMachineInstrToMCInst and RISCVInsertVSETVLI.cpp.

HsiangKai marked 2 inline comments as done.Jul 26 2021, 7:09 PM
HsiangKai retitled this revision from [PoC][RISCV] Add the tail policy argument to builtins/intrinsics. to [RISCV] Add the tail policy argument to builtins/intrinsics..Jul 26 2021, 8:25 PM
HsiangKai edited the summary of this revision. (Show Details)
HsiangKai added reviewers: khchen, arcbbb, evandro.

Update vadd-rv32.ll test case.

No update to other RVV test cases in this patch to make it reviewed easier.

craig.topper added inline comments.Sep 2 2021, 1:47 PM
llvm/include/llvm/IR/IntrinsicsRISCV.td
169

This needs ImmArg<ArgIndex<4>>. Similar for the rest. That will guarantee it is a constant and will make SelectionDAGBuilder create a target constant instead of a regular constant.

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
248 ↗(On Diff #370285)

Can we use

uint64_t Policy = Node->getConstantOperandVal(CurOp++);
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4761 ↗(On Diff #370285)

TargetConstant?

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
645

Why uimm5? I think VPseudoTernaryNoMaskWithPolicy is using ixlenimm like $sew

HsiangKai updated this revision to Diff 370440.Sep 2 2021, 4:10 PM

Address comments.

craig.topper added inline comments.Sep 3 2021, 2:18 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
2274

I think this should be timm:$policy

HsiangKai retitled this revision from [RISCV] Add the tail policy argument to builtins/intrinsics. to [RISCV] (1/2) Add the tail policy argument to builtins/intrinsics..Sep 6 2021, 6:44 AM
HsiangKai updated this revision to Diff 370980.Sep 6 2021, 7:01 PM

Use timm for the $policy argument in the patterns.

craig.topper added inline comments.Sep 7 2021, 9:14 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
2278

Does timm:$policy work down here too?

HsiangKai updated this revision to Diff 371499.Sep 8 2021, 9:11 PM

Address comments.

This revision is now accepted and ready to land.Sep 22 2021, 7:46 PM
craig.topper requested changes to this revision.Sep 22 2021, 7:48 PM
craig.topper added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td
1216

This should be TAIL_UNDISTURBED. It has a $merge operand

1217–1218

Same here

1243

And here

1288–1289

And here

1313

And here

This revision now requires changes to proceed.Sep 22 2021, 7:48 PM
craig.topper accepted this revision.Sep 22 2021, 7:52 PM

Nevermind.

LGTM

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

Err nevermind. Maybe we only cared about the mask. Probably should add the policy to riscv_vselect_vl, but that's for another patch.

This revision is now accepted and ready to land.Sep 22 2021, 7:52 PM
khchen added inline comments.Sep 23 2021, 12:05 AM
llvm/include/llvm/IR/IntrinsicsRISCV.td
162

maybe we could have another NFC patch to update those argument info comments.

I will wait for https://reviews.llvm.org/D109322 be accepted. These two patches need to get in together.

HsiangKai added inline comments.Sep 23 2021, 2:09 AM
llvm/include/llvm/IR/IntrinsicsRISCV.td
162

I will update these comments. Thanks.

This revision was landed with ongoing or failed builds.Sep 24 2021, 2:11 AM
This revision was automatically updated to reflect the committed changes.