riscv-non-isa / rvv-intrinsic-doc

https://jira.riscv.org/browse/RVG-153
BSD 3-Clause "New" or "Revised" License
284 stars 89 forks source link

Add intrinsics for xtheadvector extension #298

Closed joshua-arch1 closed 2 months ago

joshua-arch1 commented 10 months ago

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

cmuellner commented 10 months ago

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:

sequencer commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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: https://github.com/revyos/revyos/issues/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 commented 10 months ago

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:

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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.

kito-cheng commented 2 months ago

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