Skip to content
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

Closed
wants to merge 5 commits into from

Conversation

joshua-arch1
Copy link

@joshua-arch1 joshua-arch1 commented Nov 17, 2023

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

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>
@joshua-arch1 joshua-arch1 changed the title Update rvv-intrinsic-spec.adoc Add intrinsics for xtheadvector extension Nov 18, 2023
@cmuellner
Copy link
Contributor

Link to the XTheadVector spec (adoc):
https://github.com/T-head-Semi/thead-extension-spec/blob/master/xtheadvector.adoc
And the generated spec PDF:
https://github.com/T-head-Semi/thead-extension-spec/releases/download/2.3.0/xthead-2023-11-10-2.3.0.pdf

A GCC patchset that implements this API can be found here:
https://gcc.gnu.org/pipermail/gcc-patches/2023-November/637115.html

Some more comments about the reasoning behind XTheadVector can be found here:
https://gcc.gnu.org/pipermail/gcc-patches/2023-November/637139.html

XTheadVector has a huge overlap with RVV.
So it is reasonable, to reuse the RVV intrinsic API for XTheadVector as well (when applicable) and extend the API by additional functions.

We are aware that this PR raises the following generic questions:

  • Should vendor vector extensions that are somewhat compatible with RVV be covered in this document?
  • If yes, how should the integration of vector intrinsic functions for vendor extension be integrated into this repo?
    Answering this questions was also one of the intents to create this PR.
    But there is no rush to decide on that.
    And if the decision is to not have this API documented here, this will be moved to a T-Head Github repo.

@sequencer
Copy link

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!

@cmuellner
Copy link
Contributor

The RISC-V vector community is diverse enough because of different uArch(X280, X390, Andes NX27V, T-Head C908, Semidynamic Vector, T1, Ara)

There are no micro-architectural properties modeled in this repo.
So I don't understand this comment in the context of this PR.

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).

Maintaining a renamed buggy version of RVV 0.7 is really a good idea to improve community diversity!

Can you please elaborate on what you mean with "buggy"?

@sequencer
Copy link

From my understanding, the xtheadvector doesn't based on riscv-vector-spec. That's what I really concern, correct me if I'm wrong.

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).

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 De facto standard, aka RVV 0.7.1 here, in the RISC-V community. That's a really bad example and not fair to any other vendors, since they are all focusing on the ratified spec, and contributing the a same ISA, they may have their own custom instructions, but they don't explicitly fork or modify RISC-V Spec.

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.

Can you please elaborate on what you mean with "buggy"?

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 xtheadvector is more like an errata list + custom instruction combination based on the RISC-V Vector 0.7.1.

@cmuellner
Copy link
Contributor

From my understanding, the xtheadvector doesn't based on riscv-vector-spec. That's what I really concern, correct me if I'm wrong.

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).

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 De facto standard, aka RVV 0.7.1 here, in the RISC-V community. That's a really bad example and not fair to any other vendors, since they are all focusing on the ratified spec, and contributing the a same ISA, they may have their own custom instructions, but they don't explicitly fork or modify RISC-V Spec.

That is not correct. T-Head never intended to undermine any RVI standards.
T-Head has already proven to be interested to follow the ratified RVV specification.
You already named the C908 in your previous post, which implements the ratified RVV spec.
The XTheadVector specification is not intended to support future cores, but to support cores that are already available in the field (in products like the Allwinner D1, BeagleBoard BeagleV-Ahead, Sophgo Milk-V).

The question therefore is, if users of these products will benefit from upstream support, or not.

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.

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.
Some say it was bad communication from RVI, some say it was a misunderstanding of T-Head.
I'm only interested that we prevent such a situation in the future.
RVI enhanced their communication since the first T-Head chips with their vector implementation was shipped.
E.g. RVI became much stricter in the used terminology and documented the ratification processes in more details.
Also the following page has been created to remind everyone about the meaning of the specification states: https://wiki.riscv.org/display/HOME/Specification+States.
For me this is enough to believe that we won't end up in the same situation with the same reasons in another specification.

Can you please elaborate on what you mean with "buggy"?

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 xtheadvector is more like an errata list + custom instruction combination based on the RISC-V Vector 0.7.1.

The RISC-V specification does not call this a bug, but a "non-conforming" extension:

We use the term non-conforming to describe a non-standard extension that uses either a standard or a reserved encoding.

@sequencer
Copy link

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:

The question therefore is, if users of these products will benefit from upstream support, or not.

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.

@cmuellner
Copy link
Contributor

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 the details here, but this reads indeed like a bug (the implementation does not follow the specification).
That's different to the XTheadVector implementation, which implements a stable specification.
But stable does not mean, that the spec is frozen or ratified.

So basically we have these cases:

  • bug (implementation does not follow the specification)
  • non-intentional non-conforming extension
  • intentional non-conforming extension

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.

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?

RISC-V has a two custom opcode spaces.
And T-Head (like others) is using these for their vendor extensions.
So why would T-Head not use the vendor opcode space for their vector extensions as well?
I believe that's because they assumed to implement a standard extension.

So basically, that's what you said:

The question therefore is, if users of these products will benefit from upstream support, or not.

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.

Yes, the question is where to draw the line.
When looking at this as an "accident", which can't happen again (because the terms stable and frozen are much better communicated), then I would say there is a low chance that we see more non-conforming extensions asking for upstream submission in the future.

@sequencer
Copy link

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.

@nick-knight
Copy link
Collaborator

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.

@eopXD
Copy link
Collaborator

eopXD commented Nov 29, 2023

Hi @joshua-arch1,

I think there are some reasons that have me oppose to the idea of hosting vendor-defined intrinsics under this repository.
Although in the extension specification [0], it refers to the v0.7 version of the "V" extension, I do hope that such referral companied with bullet points of modification can be replaced with hosting the instructions directly under that documentation with full coverage of details. This is for the benefit of both the T-head extension users and users of the "V" extension to avoid confusion.

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 vendor-inst interface. PR is welcomed to improve the generator too!

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

@cmuellner
Copy link
Contributor

@eopXD, I was discussing this today with T-Head, and we agreed to move this into a T-Head repository.
Nick already gave a reasonable comment to clarify why vendor vector extension intrinsic APIs should not be documented in this repo.

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.

@eopXD
Copy link
Collaborator

eopXD commented Nov 29, 2023

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.

Comment on lines +785 to +788
#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>
Copy link

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, thanks for reporting!

ChunyuLiao pushed a commit to ruyisdk/llvm-project that referenced this pull request Dec 4, 2023
* [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
imkiva added a commit to imkiva/llvm-project that referenced this pull request Apr 1, 2024
* [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
@kito-cheng
Copy link
Collaborator

Gonna close that, I am OK with either list vendor extension in README or just listed in riscv-toolchain-conventions, feel free to create another PR if you would like listed in the README in this repo :)

@kito-cheng kito-cheng closed this Jul 15, 2024
imkiva added a commit to imkiva/llvm-project that referenced this pull request Feb 24, 2025
* [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
imkiva added a commit to imkiva/llvm-project that referenced this pull request Feb 26, 2025
* [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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants