-
Notifications
You must be signed in to change notification settings - Fork 92
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
Add intrinsics for xtheadvector extension #298
Conversation
Add xtheadvector intrinsics Signed-off-by: joshua-arch1 <68843032+joshua-arch1@users.noreply.github.com>
Signed-off-by: joshua-arch1 <68843032+joshua-arch1@users.noreply.github.com>
Signed-off-by: joshua-arch1 <68843032+joshua-arch1@users.noreply.github.com>
Signed-off-by: joshua-arch1 <68843032+joshua-arch1@users.noreply.github.com>
Signed-off-by: joshua-arch1 <68843032+joshua-arch1@users.noreply.github.com>
Link to the XTheadVector spec (adoc): A GCC patchset that implements this API can be found here: Some more comments about the reasoning behind XTheadVector can be found here: XTheadVector has a huge overlap with RVV. We are aware that this PR raises the following generic questions:
|
The RISC-V vector community is diverse enough because of different uArch(X280, X390, Andes NX27V, T-Head C908, Semidynamic Vector, T1, Ara) Maintaining a renamed buggy version of RVV 0.7 is really a good idea to improve community diversity! |
There are no micro-architectural properties modeled in this repo. I also wonder why members of the RISC-V vector community would complain about too many different implementations of RVV. Especially, since many of them are part of organizations that have their own micro-architecture (and therefore contribute to this situation).
Can you please elaborate on what you mean with "buggy"? |
From my understanding, the
I'm not complaining about supporting different uarch is bad or burden. The riscv-vector-spec allows and encourage us doing so. This is a good thing that the community all agree. rvv-intrinsic is uarch agnostic, so all downstream uarch can share it, and that's what it used for. What I against it is: A RVI member company tries to establish a Custom instructions are good, but custom instructions should live in the standard custom instruction space. There are a lot of different custom instructions, e.g. RoCC from RocketChip, SCIE and VCIX from SiFive, CHERI From Cambridge, that's what RISC-V diversity should really mean. While T-HEAD tries to push an unratified spec(RVV 0.7.1) to community and call out "we tapeout, I should upstream". This is not a story a community can take. Basically, it is not a custom instruction set, it takes the instruction space that occupied by the standard RISC-V Vector 1.0.
Buggy here means this vendor forked RISC-V Vector 0.7.1 as their new custom instruction, the difference between RVV 0.7.1 is what I called bug here, the |
That is not correct. T-Head never intended to undermine any RVI standards. The question therefore is, if users of these products will benefit from upstream support, or not.
I'm not interested much in the reasons why we ended up the situation with the V 0.7.1 spec being shipped in a device.
The RISC-V specification does not call this a
|
Sorry for calling it buggy, it's too aggressive, I apology. But the reason I become so aggressive to T-HEAD, is I can remember last time some arbitrary software engineer asked me about T-HEAD's another "non-conforming" issue: revyos/revyos#17. OK, T-HEAD even "non-conform" to FD. I don't know, how many "non-conforming" issues that could be brought by T-HEAD in the future. But as T1's architect, what I concern about is, whether software will support these "non-conforming" behavior, and in the future, when we touch the code base and try to do some contribution, should we also make our contribution comply to these "non-conforming" behavior? So basically, that's what you said:
As a current user to rvv-intrinsic, I just express my concern: Will the upstream software becoming too complex just because supporting "non-conforming" architectures? I hope not, but that's not what I can control. What I can do is providing the conforming implementation as much as we can. |
I don't know the details here, but this reads indeed like a bug (the implementation does not follow the specification). So basically we have these cases:
The reason why I asked about the "buggy" part is, that I initially thought you are talking about some bugs in XTheadVector (w.r.t RVV 0.7.1), which I am not aware of.
RISC-V has a two custom opcode spaces.
Yes, the question is where to draw the line. |
I used to test the board you mentioned, "Allwinner D1, BeagleBoard BeagleV-Ahead, Sophgo Milk-V". IMHO, they are far away from mass production, I don't know should I blame T-HEAD or the chip vendor. But, AFAIK, Sophgo decides to switch to SiFive in their next generation, known as SG2380. I doubt whether there are enough users, so upstream should take the burden from T-HEAD's for these products. But this is just concerns from downstream project developer, let the final decision made by the upstream developers. |
I have no objection to T-Head defining intrinsics for their non-conforming or custom extensions. I do believe this is beyond the charter of the RVV intrinsics task group, and does not belong in this repo. (I suggest asking the SW HC.) I feel that T-head should maintain their API documentation separately, like they already do for their custom ISA extensions. There may be work required over at the (RVI-sponsored) C API or Toolchain Conventions repos. |
Hi @joshua-arch1, I think there are some reasons that have me oppose to the idea of hosting vendor-defined intrinsics under this repository. I see the T-Head extension as a vendor extension to a vector architecture, buggy or not aside, standing next to the RISC-V ratified, consented "V" extension. The same concept follows along to intrinsics. This repository focus on defining the extensions that go through the ratification process of RISC-V International. The generator is designed to be reusable, as the BFloat16 PR #293 demonstrates so. I encourage others to reuse it and will be happy to provide advices. Custom intrinsics prototype list and test cases can be defined using the generator as submodule and utilizing the I understand that people do come here first to find out what intrinsics are available, but adding description in the v1.0 specification will cause confusion. Additionally, we do not want to modify a ratified specification everytime a new set of vendor intrinsic comes up. As a community, I recommend to host a section under the README of this repository to point to the extension specification, intrinsic specification, and upstream toolchain release(s) available for them for vendor extensions. Let us discuss this further in the next intrinsics monthly meeting. Please do try to attend, as I know having a public accessible specification is important. [0] https://github.com/T-head-Semi/thead-extension-spec/blob/master/xtheadvector.adoc |
@eopXD, I was discussing this today with T-Head, and we agreed to move this into a T-Head repository. I don't oppose the proposed list of links to vendor extension resources in the readme, but it might be some headache to maintain that. |
Thank you for sorting this out @cmuellner. I believe it will be the vendors' responsibility to expose their own intrinsics provided. So if they want a pointer displayed in the README of this repo, I do not think it is a bad idea. I think it will be on the vendor side to provide the correct information here. So I think a disclaimer of so (that these pointers are provided and maintained by the vendor) added to the vendor section of the README in this repo will resolve the maintenance responsibility issue and delegate them to the vendors who is ultimately responsible for them. |
#if defined(__riscv_vector) && defined(__riscv_v_intrinsics) | ||
#include <riscv_vector.h> | ||
#elif defined (__riscv_xtheadvector) && defined (__riscv_th_v_intrinsics) | ||
#include <riscv_th_vector.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.
Possible typo here: __riscv_th_v_intrinsics
should be __riscv_th_v_intrinsic
.
Same for __riscv_v_intrinsics
, which should be __riscv_v_intrinsic
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.
Correct, thanks for reporting!
* [LLVM][RVV 0.7.1] `xtheadv` -> `xtheadvector` * [LLVM][RVV 0.7.1] MC names (1/N) * [LLVM][RVV 0.7.1] MC names (2/N) * [LLVM][RVV 0.7.1] MC names (3/N) * [LLVM][RVV 0.7.1] MC names (4/N) * [LLVM][RVV 0.7.1] LLVM intrinsic names (5/N) * [LLVM][RVV 0.7.1] LLVM intrinsic names (6/N) * [LLVM][RVV 0.7.1] Pass all MC tests * [LLVM][RVV 0.7.1] Pass all LLVM CodeGen tests * [LLVM][RVV 0.7.1] `llvm.riscv.xv*` -> `llvm.riscv.th.v*` * [Clang][RVV 0.7.1] `xv*` -> `th_v*` * [Clang][RVV 0.7.1] Update tests * [Clang][RVV 0.7.1] Update macros and headers with T-Head upstream Ref: riscv-non-isa/rvv-intrinsic-doc#298 * [LLVM][RVV 0.7.1] Rename * [Clang][RVV 0.7.1] Fix pragma * [LLVM][RVV 0.7.1] Fix vleff tests
* [LLVM][RVV 0.7.1] `xtheadv` -> `xtheadvector` * [LLVM][RVV 0.7.1] MC names (1/N) * [LLVM][RVV 0.7.1] MC names (2/N) * [LLVM][RVV 0.7.1] MC names (3/N) * [LLVM][RVV 0.7.1] MC names (4/N) * [LLVM][RVV 0.7.1] LLVM intrinsic names (5/N) * [LLVM][RVV 0.7.1] LLVM intrinsic names (6/N) * [LLVM][RVV 0.7.1] Pass all MC tests * [LLVM][RVV 0.7.1] Pass all LLVM CodeGen tests * [LLVM][RVV 0.7.1] `llvm.riscv.xv*` -> `llvm.riscv.th.v*` * [Clang][RVV 0.7.1] `xv*` -> `th_v*` * [Clang][RVV 0.7.1] Update tests * [Clang][RVV 0.7.1] Update macros and headers with T-Head upstream Ref: riscv-non-isa/rvv-intrinsic-doc#298 * [LLVM][RVV 0.7.1] Rename * [Clang][RVV 0.7.1] Fix pragma * [LLVM][RVV 0.7.1] Fix vleff tests
Gonna close that, I am OK with either list vendor extension in README or just listed in |
* [LLVM][RVV 0.7.1] `xtheadv` -> `xtheadvector` * [LLVM][RVV 0.7.1] MC names (1/N) * [LLVM][RVV 0.7.1] MC names (2/N) * [LLVM][RVV 0.7.1] MC names (3/N) * [LLVM][RVV 0.7.1] MC names (4/N) * [LLVM][RVV 0.7.1] LLVM intrinsic names (5/N) * [LLVM][RVV 0.7.1] LLVM intrinsic names (6/N) * [LLVM][RVV 0.7.1] Pass all MC tests * [LLVM][RVV 0.7.1] Pass all LLVM CodeGen tests * [LLVM][RVV 0.7.1] `llvm.riscv.xv*` -> `llvm.riscv.th.v*` * [Clang][RVV 0.7.1] `xv*` -> `th_v*` * [Clang][RVV 0.7.1] Update tests * [Clang][RVV 0.7.1] Update macros and headers with T-Head upstream Ref: riscv-non-isa/rvv-intrinsic-doc#298 * [LLVM][RVV 0.7.1] Rename * [Clang][RVV 0.7.1] Fix pragma * [LLVM][RVV 0.7.1] Fix vleff tests
* [LLVM][RVV 0.7.1] `xtheadv` -> `xtheadvector` * [LLVM][RVV 0.7.1] MC names (1/N) * [LLVM][RVV 0.7.1] MC names (2/N) * [LLVM][RVV 0.7.1] MC names (3/N) * [LLVM][RVV 0.7.1] MC names (4/N) * [LLVM][RVV 0.7.1] LLVM intrinsic names (5/N) * [LLVM][RVV 0.7.1] LLVM intrinsic names (6/N) * [LLVM][RVV 0.7.1] Pass all MC tests * [LLVM][RVV 0.7.1] Pass all LLVM CodeGen tests * [LLVM][RVV 0.7.1] `llvm.riscv.xv*` -> `llvm.riscv.th.v*` * [Clang][RVV 0.7.1] `xv*` -> `th_v*` * [Clang][RVV 0.7.1] Update tests * [Clang][RVV 0.7.1] Update macros and headers with T-Head upstream Ref: riscv-non-isa/rvv-intrinsic-doc#298 * [LLVM][RVV 0.7.1] Rename * [Clang][RVV 0.7.1] Fix pragma * [LLVM][RVV 0.7.1] Fix vleff tests
This is a proposal for XTheadVector C intrinsics that provide users interfaces in the C language level to directly leverage https://github.com/T-head-Semi/thead-extension-spec/[XTheadVector specification].
@cmuellner
@eopXD
@JeffreyALaw
@kito-cheng
@palmer-dabbelt
@ptomsich