Page MenuHomePhabricator

[PoC][RISCV] Use an attribute to declare C intrinsics with different policy.
Needs ReviewPublic

Authored by HsiangKai on Oct 26 2021, 6:47 AM.

Details

Summary

This patch provides a proof-of-concept implementation of the proposal. https://github.com/riscv-non-isa/rvv-intrinsic-doc/issues/120

In this patch, we create a new attribute rvv_policy to annotate C
intrinsics with its tail/inactive elements policy. The syntax is

__attribute__((rvv_policy(tama)))
vint32m1_t vadd_vv_i8m1_tama(...);

The possible policies are tama, tamu, tuma, tumu.
ta: tail agnostic
tu: tail undisturbed
ma: inactive masked-off agnostic
mu: inactive masked-off undisturbed

The attribute is used in riscv_vector.h. This implementation will not increase the number of clang builtins.

Diff Detail

Unit TestsFailed

TimeTest
40 msx64 debian > Clang.Misc::pragma-attribute-supported-attributes-list.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang-tblgen -gen-clang-test-pragma-attribute-supported-attributes -I/var/lib/buildkite-agent/builds/llvm-project/clang/include /var/lib/buildkite-agent/builds/llvm-project/clang/include/clang/Basic/Attr.td -o - | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/clang/test/Misc/pragma-attribute-supported-attributes-list.test

Event Timeline

HsiangKai created this revision.Oct 26 2021, 6:47 AM
HsiangKai requested review of this revision.Oct 26 2021, 6:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 26 2021, 6:47 AM
HsiangKai updated this revision to Diff 382298.Oct 26 2021, 6:48 AM

Remove redundant test case.

HsiangKai edited the summary of this revision. (Show Details)Oct 26 2021, 6:51 AM
HsiangKai edited the summary of this revision. (Show Details)
HsiangKai edited the summary of this revision. (Show Details)
HsiangKai edited the summary of this revision. (Show Details)Oct 26 2021, 6:54 AM
craig.topper added inline comments.Oct 26 2021, 2:33 PM
clang/lib/CodeGen/CGBuiltin.cpp
18614

Why size_t? This would be the size_t of the host machine that's building/running the compiler and would have no connection to the architecture being targetted.

clang/utils/TableGen/RISCVVEmitter.cpp
843

Do we need to emit this switch for every builtin? Couldn't we assign PolicyValue before including the autogenerated file and only builtins that have a policy would add PolicyValue it to their Ops vector?

916

Does the InputTypes.empty() check provide any value. Looks like it just prevents constructing a ListSeparator that wouldn't be used, but I would think that's cheap to construct. I know this was copied from the function above, so my question applies there too.

Address @craig.topper's comments.

HsiangKai marked 3 inline comments as done.Oct 26 2021, 11:23 PM

I think the concept seems good to me. I'd like @aaron.ballman to review the attribute code.

clang/lib/CodeGen/CGBuiltin.cpp
18610

Is constant still used?

clang/utils/TableGen/RISCVVEmitter.cpp
883

Capitalize policySuffix and make it

static const char *const PolicySuffix[]

Just nits from me at this stage.

clang/include/clang/Basic/AttrDocs.td
2150

Nit, but the use of could seems out of place in this documentation. Is can or may perhaps more common?

clang/utils/TableGen/RISCVVEmitter.cpp
895

This variable i shadowing the outer loop's induction variable is a little odd. Perhaps the outer loop could use a range-based for?

luke957 resigned from this revision.Oct 28 2021, 7:29 AM

So sorry for my bad herald script.