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.



This patch provides a proof-of-concept implementation of the proposal.

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

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

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/ -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

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.


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?


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.


Is constant still used?


Capitalize policySuffix and make it

static const char *const PolicySuffix[]

Just nits from me at this stage.


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


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.