-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Intrinsics: Define intrinsics naming guidelines #31
Conversation
CC @kito-cheng @asb |
CC @eopXD |
riscv-c-api.md
Outdated
`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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sequenzes
-> sequences
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
riscv-c-api.md
Outdated
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably missing an optional naming rule on "type abbreviation" (e.g. the i
here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add. Thanks!
@ptomsich mentioned in today's SIG Toolchain call that he prefers |
Shall we need to consider about customized or vendor's intrinsic name ? |
b447428
to
2440512
Compare
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! |
Regarding the vendor instructions, I don't see why we need a vendor prefix because the instructions are already prefixed (e.g. |
Generally LGTM :)
|
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 |
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. 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:
|
2440512
to
6687ecd
Compare
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`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any rules about the header file naming if header file required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, and it will be good to have same filename naming rule for different toolchains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Giving my blessing of this PR, hope this could push this forward :)
Thanks for the approval! |
Let's establish a naming guideline for intrinsics. Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
6687ecd
to
b1447c8
Compare
I got the feedback that the CMO examples are confusing. Therefore I dropped them. |
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/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/rvv-intrinsic-doc#189 Depends on D142085. Reviewed By: kito-cheng Differential Revision: https://reviews.llvm.org/D142644
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/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/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 <eop.chen@sifive.com>
…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/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/rvv-intrinsic-doc#189 Depends on D142085. Reviewed By: kito-cheng Differential Revision: https://reviews.llvm.org/D142644
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/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/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/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/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/rvv-intrinsic-doc#189 Depends on D142644. Reviewed By: kito-cheng Differential Revision: https://reviews.llvm.org/D142697
I'm hesitant to say anything, because adding the |
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. |
The
We explicitly dropped type prefixes, because there was no obvious consensus.
As mentioned by Kito:
|
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.