Intrinsics: Define intrinsics naming guidelines by cmuellner · Pull Request #31 · riscv-non-isa/riscv-c-api-doc

cmuellner · 2022-11-21T14:54:29Z

Let's establish a naming guideline for intrinsics.

Contrary to the previous attempt in #25, this PR does not try to make a guideline that covers all existing formats but defines a single new format.

cmuellner · 2022-11-21T14:55:10Z

CC @kito-cheng @asb

cmuellner · 2022-11-21T14:58:49Z

CC @eopXD

eopXD · 2022-11-21T15:09:14Z

-`gcc/gcc/config/riscv/riscv-builtins.c`...
+Intrinsic functions (or intrinsics or built-ins) are functions that are expanded into instruction sequences by compilers.
+They typically provide access to functionality that is otherwise not synthesizable by compilers.
+Some intrinsics expand to different code sequenzes, depending on the available instructions from the enabled ISA extensions.


sequenzes -> sequences

eopXD · 2022-11-21T15:39:06Z

+int32_t __rv_orc_b_32(int32_t rs);
+
+vadd.vv vd, vs2, vs1:
+vint8m1_t __rv_vadd_vv_i8m1(vint8m1_t vs2, vint8m1_t vs1, size_t vl);


Probably missing an optional naming rule on "type abbreviation" (e.g. the i here)

Will add. Thanks!

cmuellner · 2022-11-21T15:45:55Z

@ptomsich mentioned in today's SIG Toolchain call that he prefers __riscv_ as prefix over __rv_. There were no objections, so I'll update the PR accordingly.

rjiejie · 2022-11-23T01:55:29Z

Shall we need to consider about customized or vendor's intrinsic name ?
like __riscv_<vendor>_ ?
e.g. __riscv_thead_

cmuellner · 2022-11-24T15:25:22Z

I've updated the PR according to the feedback so far.

Regarding the vendor-specific intrinsics, I have no opinion yet. But I think the proposal sounds reasonable. Thanks!

cmuellner · 2022-11-24T16:20:47Z

Regarding the vendor instructions, I don't see why we need a vendor prefix because the instructions are already prefixed (e.g. th.mula -> __riscv_th_mula). Do you think we need more?

kito-cheng · 2022-12-01T15:34:43Z

Generally LGTM :)

Regarding the vendor instructions, I don't see why we need a vendor prefix because the instructions are already prefixed (e.g. th.mula -> __riscv_th_mula). Do you think we need more?

__riscv_th or __riscv_thead are both valid on current scheme I think.

cmuellner · 2022-12-01T16:13:26Z

__riscv_th or __riscv_thead are both valid on current scheme I think.

I think that all vendor-specific intrinsics would only be covering vendor-instructions. In this case, the instruction's name is the preferred choice for the name. Therefore __riscv_th would be used for T-Head-specific intrinsics.

rjiejie · 2022-12-08T07:42:02Z

Regarding the vendor instructions, I don't see why we need a vendor prefix because the instructions are already prefixed (e.g. th.mula -> __riscv_th_mula). Do you think we need more?

Looks like it's a choice although the prefix 'th' have no obvious meaning about vendor's info in user side :)

cmuellner · 2022-12-08T09:09:50Z

Regarding the vendor instructions, I don't see why we need a vendor prefix because the instructions are already prefixed (e.g. th.mula -> __riscv_th_mula). Do you think we need more?

Looks like it's a choice although the prefix 'th' have no obvious meaning about vendor's info in user side :)

The vendor prefixes are required for vendor instructions.
Therefore they are standardized in the riscv-toolchain-conventions: https://github.com/riscv-non-isa/riscv-toolchain-conventions/blob/master/README.mkd#list-of-vendor-prefixes

E.g. in Binutils all instructions of the T-Head vendor extensions have the prefix "th" (e.g. "th.ldd" which is part of XTheadMemPair).

I'll add the following sentence:

Note, that intrinsics that are restricted to RISC-V vendor extensions need to include the vendor-prefix (as documented in the RISC-V toolchain conventions).

fanghuaqi · 2022-12-22T07:50:24Z

+Providing functionality via architecture-independent intrinsics is the preferred method, as it improves code portability.
+
+Some intrinsics are only available if a particular header file is included.
+RISC-V header files that enable intrinsics require the prefix `riscv_` (e.g. `riscv_vector.h` or `riscv_crypto.h`).


Is there any rules about the header file naming if header file required?

Short answer: no rules beyond the prefix riscv_.

The reason is that this guide cannot foresee which grouping makes sense.
E.g. vector crypto instructions might be gated by riscv_vector.h, riscv_vector_crypto.h, or riscv_crypto.h. All of them would be valid candidates (maybe even multiple options), so this will have to be decided on a case-by-case basis.

Got it, and it will be good to have same filename naming rule for different toolchains.

kito-cheng

Giving my blessing of this PR, hope this could push this forward :)

cmuellner · 2023-01-30T14:27:54Z

Thanks for the approval!
I'll merge this later today.

Let's establish a naming guideline for intrinsics. Signed-off-by: Christoph Müllner <[email protected]>

cmuellner · 2023-01-30T14:41:04Z

I got the feedback that the CMO examples are confusing. Therefore I dropped them.

cmuellner · 2023-01-30T16:04:01Z

This was discussed (again) in today's SIG Toolchain call, and there were no objections to this proposal.

…setvl, and vsetvlmax This commit adds prefix for intrinsics that are defined through `HeaderCode` under `riscv_vector.td`. This is the 1st commit of a patch-set to add `__riscv_` for all RVV intrinsics. This follows the naming guideline under riscv-c-api-doc to add the `__riscv_` suffix for all RVV intrinsics. Pull Request: riscv-non-isa/riscv-c-api-doc#31 riscv-non-isa/riscv-rvv-intrinsic-doc#189 Depends on D142016. Reviewed By: kito-cheng Differential Revision: https://reviews.llvm.org/D142085

This commit adds prefix for the non-overloaded RVV intrinsics. This is the 2nd commit of a patch-set to add __riscv_ for all RVV intrinsics. This follows the naming guideline under riscv-c-api-doc to add the `__riscv_` suffix for all RVV intrinsics. Pull Request: riscv-non-isa/riscv-c-api-doc#31 riscv-non-isa/riscv-rvv-intrinsic-doc#189 Depends on D142085. Reviewed By: kito-cheng Differential Revision: https://reviews.llvm.org/D142644

This commit adds prefix for the overloaded RVV intrinsics. This is the 3rd commit of a patch-set to add __riscv_ for all RVV intrinsics. This follows the naming guideline under riscv-c-api-doc to add the `__riscv_` suffix for all RVV intrinsics. Pull Request: riscv-non-isa/riscv-c-api-doc#31 riscv-non-isa/riscv-rvv-intrinsic-doc#189 Depends on D142644. Reviewed By: kito-cheng Differential Revision: https://reviews.llvm.org/D142697

Linking to riscv-non-isa/riscv-c-api-doc#31 Signed-off-by: eop Chen <[email protected]>

…setvl, and vsetvlmax This commit adds prefix for intrinsics that are defined through `HeaderCode` under `riscv_vector.td`. This is the 1st commit of a patch-set to add `__riscv_` for all RVV intrinsics. This follows the naming guideline under riscv-c-api-doc to add the `__riscv_` suffix for all RVV intrinsics. Pull Request: riscv-non-isa/riscv-c-api-doc#31 riscv-non-isa/riscv-rvv-intrinsic-doc#189 Depends on D142016. Reviewed By: kito-cheng Differential Revision: https://reviews.llvm.org/D142085

This commit adds prefix for the non-overloaded RVV intrinsics. This is the 2nd commit of a patch-set to add __riscv_ for all RVV intrinsics. This follows the naming guideline under riscv-c-api-doc to add the `__riscv_` suffix for all RVV intrinsics. Pull Request: riscv-non-isa/riscv-c-api-doc#31 riscv-non-isa/riscv-rvv-intrinsic-doc#189 Depends on D142085. Reviewed By: kito-cheng Differential Revision: https://reviews.llvm.org/D142644

This commit adds prefix for the overloaded RVV intrinsics. This is the 3rd commit of a patch-set to add __riscv_ for all RVV intrinsics. This follows the naming guideline under riscv-c-api-doc to add the `__riscv_` suffix for all RVV intrinsics. Pull Request: riscv-non-isa/riscv-c-api-doc#31 riscv-non-isa/riscv-rvv-intrinsic-doc#189 Depends on D142644. Reviewed By: kito-cheng Differential Revision: https://reviews.llvm.org/D142697

nick-knight · 2023-03-10T03:32:54Z

I'm hesitant to say anything, because adding the __riscv_ prefix to our vector intrinsics across the codebase has been a huge productivity killer this week. And it makes the already-lengthy vector intrinsics even more annoying to type. But I just want to confirm that you aren't also planning to require some kind of prefix for the RVV intrinsics types (vint8m8_t, etc.).

Firdous24 · 2023-03-26T11:57:20Z

Which version/release/branch of riscv-gnu-toolchain supports the prefix "_riscv" in RVV intrinsic? As I have cloned the latest version of toolchain but it doesn't support the _riscv prefix.

cmuellner · 2023-03-27T08:46:18Z

I'm hesitant to say anything, because adding the __riscv_ prefix to our vector intrinsics across the codebase has been a huge productivity killer this week. And it makes the already-lengthy vector intrinsics even more annoying to type.

The __riscv_ prefix was agreed upon before merging this PR with the toolchain community.
This was not done to kill productivity but to avoid namespace conflicts in the future.
But I appreciate the required work to change code that uses the intrinsics.
We should have defined this earlier.

But I just want to confirm that you aren't also planning to require some kind of prefix for the RVV intrinsics types (vint8m8_t, etc.).

We explicitly dropped type prefixes, because there was no obvious consensus.
Therefore this has to be worked out by the RVV intrinsics TG as part of the RVV intrinsics spec.

Which version/release/branch of riscv-gnu-toolchain supports the prefix "_riscv" in RVV intrinsic? As I have cloned the latest version of toolchain but it doesn't support the _riscv prefix.

As mentioned by Kito:

GCC 13 will support latest intrinsic, which will release at end of April in general, you can switch gcc to master branch if you want to get a try now.

…setvl, and vsetvlmax This commit adds prefix for intrinsics that are defined through `HeaderCode` under `riscv_vector.td`. This is the 1st commit of a patch-set to add `__riscv_` for all RVV intrinsics. This follows the naming guideline under riscv-c-api-doc to add the `__riscv_` suffix for all RVV intrinsics. Pull Request: riscv-non-isa/riscv-c-api-doc#31 riscv-non-isa/riscv-rvv-intrinsic-doc#189 Depends on D142016. Reviewed By: kito-cheng Differential Revision: https://reviews.llvm.org/D142085

This commit adds prefix for the non-overloaded RVV intrinsics. This is the 2nd commit of a patch-set to add __riscv_ for all RVV intrinsics. This follows the naming guideline under riscv-c-api-doc to add the `__riscv_` suffix for all RVV intrinsics. Pull Request: riscv-non-isa/riscv-c-api-doc#31 riscv-non-isa/riscv-rvv-intrinsic-doc#189 Depends on D142085. Reviewed By: kito-cheng Differential Revision: https://reviews.llvm.org/D142644

This commit adds prefix for the overloaded RVV intrinsics. This is the 3rd commit of a patch-set to add __riscv_ for all RVV intrinsics. This follows the naming guideline under riscv-c-api-doc to add the `__riscv_` suffix for all RVV intrinsics. Pull Request: riscv-non-isa/riscv-c-api-doc#31 riscv-non-isa/riscv-rvv-intrinsic-doc#189 Depends on D142644. Reviewed By: kito-cheng Differential Revision: https://reviews.llvm.org/D142697

…setvl, and vsetvlmax This commit adds prefix for intrinsics that are defined through `HeaderCode` under `riscv_vector.td`. This is the 1st commit of a patch-set to add `__riscv_` for all RVV intrinsics. This follows the naming guideline under riscv-c-api-doc to add the `__riscv_` suffix for all RVV intrinsics. Pull Request: riscv-non-isa/riscv-c-api-doc#31 riscv-non-isa/riscv-rvv-intrinsic-doc#189 Depends on D142016. Reviewed By: kito-cheng Differential Revision: https://reviews.llvm.org/D142085

This commit adds prefix for the non-overloaded RVV intrinsics. This is the 2nd commit of a patch-set to add __riscv_ for all RVV intrinsics. This follows the naming guideline under riscv-c-api-doc to add the `__riscv_` suffix for all RVV intrinsics. Pull Request: riscv-non-isa/riscv-c-api-doc#31 riscv-non-isa/riscv-rvv-intrinsic-doc#189 Depends on D142085. Reviewed By: kito-cheng Differential Revision: https://reviews.llvm.org/D142644

This commit adds prefix for the overloaded RVV intrinsics. This is the 3rd commit of a patch-set to add __riscv_ for all RVV intrinsics. This follows the naming guideline under riscv-c-api-doc to add the `__riscv_` suffix for all RVV intrinsics. Pull Request: riscv-non-isa/riscv-c-api-doc#31 riscv-non-isa/riscv-rvv-intrinsic-doc#189 Depends on D142644. Reviewed By: kito-cheng Differential Revision: https://reviews.llvm.org/D142697

eopXD reviewed Nov 21, 2022

View reviewed changes

cmuellner force-pushed the intrinsics2 branch from b447428 to 2440512 Compare November 24, 2022 15:23

cmuellner requested a review from kito-cheng December 1, 2022 15:11

cmuellner force-pushed the intrinsics2 branch from 2440512 to 6687ecd Compare December 8, 2022 09:10

fanghuaqi reviewed Dec 22, 2022

View reviewed changes

eopXD mentioned this pull request Jan 5, 2023

Expose vlenb to user riscv-non-isa/riscv-rvv-intrinsic-doc#182

Merged

eopXD mentioned this pull request Jan 19, 2023

Add __riscv_ prefix for all intrinsics riscv-non-isa/riscv-rvv-intrinsic-doc#189

Merged

kito-cheng approved these changes Jan 30, 2023

View reviewed changes

Intrinsics: Define intrinsics naming guidelines

b1447c8

Let's establish a naming guideline for intrinsics. Signed-off-by: Christoph Müllner <[email protected]>

cmuellner force-pushed the intrinsics2 branch from 6687ecd to b1447c8 Compare January 30, 2023 14:34

cmuellner merged commit dccfa30 into riscv-non-isa:master Jan 30, 2023

eopXD added a commit to riscv-non-isa/riscv-rvv-intrinsic-doc that referenced this pull request Feb 1, 2023

Add __riscv_ prefix for all intrinsics

efb9e56

Linking to riscv-non-isa/riscv-c-api-doc#31 Signed-off-by: eop Chen <[email protected]>

nick-knight mentioned this pull request Apr 19, 2023

Add sifive_x280 configuration flame/blis#737

Merged

XYenChi mentioned this pull request Oct 30, 2023

[RISCV] Porting riscv-p-ext. plctlab/riscv-gcc#4

Merged

Conversation

cmuellner commented Nov 21, 2022

Uh oh!

cmuellner commented Nov 21, 2022

Uh oh!

cmuellner commented Nov 21, 2022

Uh oh!

eopXD Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

cmuellner Nov 24, 2022

Choose a reason for hiding this comment

Uh oh!

eopXD Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmuellner Nov 24, 2022

Choose a reason for hiding this comment

Uh oh!

cmuellner commented Nov 21, 2022

Uh oh!

rjiejie commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmuellner commented Nov 24, 2022

Uh oh!

cmuellner commented Nov 24, 2022

Uh oh!

kito-cheng commented Dec 1, 2022

Uh oh!

cmuellner commented Dec 1, 2022

Uh oh!

rjiejie commented Dec 8, 2022

Uh oh!

cmuellner commented Dec 8, 2022

Uh oh!

fanghuaqi Dec 22, 2022

Choose a reason for hiding this comment

Uh oh!

cmuellner Jan 30, 2023

Choose a reason for hiding this comment

Uh oh!

fanghuaqi Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

kito-cheng left a comment

Choose a reason for hiding this comment

Uh oh!

cmuellner commented Jan 30, 2023

Uh oh!

cmuellner commented Jan 30, 2023

Uh oh!

cmuellner commented Jan 30, 2023

Uh oh!

nick-knight commented Mar 10, 2023

Uh oh!

Firdous24 commented Mar 26, 2023

Uh oh!

cmuellner commented Mar 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

eopXD Nov 21, 2022 •

edited

Loading

rjiejie commented Nov 23, 2022 •

edited

Loading