search

riscv-non-isa / rvv-intrinsic-doc

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

Add 0.11 -> 0.12 compatibility headers #255

Open mshabunin opened 1 year ago

mshabunin commented 1 year ago

Since v 0.12 has introduced 0.11-incompatible changes, it would be nice to have compatibility headers similar to 0.10 -> 0.11 (https://github.com/riscv-non-isa/rvv-intrinsic-doc/tree/main/auto-generated/rvv-v0p10-compatible-headers) to ease user transitioning.

One of related PRs: #222

vladimir-dudnik-1 commented 1 year ago

maintaining compatibility across versions of intrinsics specification is especially important for libraries which actively being developed using available RVV 1.0 intrinsics specification. For example, OpenCV build has been impacted this incompatible change and adopting relatively big code base to this change require some effort from library maintainers or community,

nick-knight commented 1 year ago

available RVV 1.0 intrinsics specification

Agreed that compatibility is an issue, and churn is painful. But the intrinsics spec hasn't yet reached "1.0", or frozen, so users should be prepared for changes. This is the same for any in-development, not-yet-ratified RISC-V thing.

asb commented 1 year ago

available RVV 1.0 intrinsics specification

Agreed that compatibility is an issue, and churn is painful. But the intrinsics spec hasn't yet reached "1.0", or frozen, so users should be prepared for changes. This is the same for any in-development, not-yet-ratified RISC-V thing.

Except we made the mistake of shipping the unfinalised intrinsics API in Clang/LLVM when moving the vector extension from experimental (I think we hadn't really considered the impact sufficiently), so there is a bit of a difference vs not-yet-ratified extensions due to this error.

nick-knight commented 1 year ago

Several of these GitHub issue threads mention "intrinsics 1.0", which does not yet exist. There seems to be persistent confusion between the ISA extension --- which was ratified (thus reaching 1.0) in November 2021 --- and its associated C-language support, which is not yet ratified.

Your comment suggests to me that a similar confusion may exist in the tools. One idea is to require "experimental" flags when using the intrinsics. Another idea is to warn the programmer when using intrinsics without an experimental flag. A third idea is to introduce a new, "experimental-intrinsics" flag. I'm sure others have more ideas. But I think this discussion may be more relevant over at llvm-project than in the API doc.

sunshaoce commented 1 year ago

May I ask, what is stopping us from upgrading to 1.0?

nick-knight commented 1 year ago

I don't understand the question. What does "1.0" mean to you?

sunshaoce commented 1 year ago

I don't understand the question. What does "1.0" mean to you?

I'm sorry, I didn't explain it clearly. What I mean is: RVV 1.0 intrinsics specification.

nick-knight commented 1 year ago

When the intrinsics specification is frozen to be submitted to RISC-V International for consideration for ratification, we will increment the version number to 1.0

mshabunin commented 1 year ago

I'm sorry if I've been not quite clear. I absolutely did not mean that compatibility is critical to us and we request these headers. We understand that it all is in an experimental state at this moment and we are grateful for your efforts in making intrinsics convenient and concise! From our side we are trying to regularly test current compilers with our codebase and increase the quality of RVV support in them.

This ticket was meant to be just a reminder, don't hesitate to close it if you do not think it is applicable. My arguments for adding compatibility headers to this repository are as follows:

We are going to implement compatibility header on our side sooner or later (probably after LLVM 17 release), but we will not cover all changed intrinsics - only those that we use in the project, thus our implementation will not be useful for others.

As for showing compiler warnings or requiring experimental flags, I don't think it is necessary. There is a warning in the spec, probably adding similar note to the README file in this repository would be enough. Currently it only states that "v0.11 does not have ... something ...", perhaps in this form it is not clear that changes between versions are incompatible.

vladimir-dudnik-1 commented 1 year ago

@nick-knight there was a wrong formulation from my side "using available RVV 1.0 intrinsics specification", but you catch up the idea correctly. I meant, as we have RVV ISA 1.0 ratified and available in tools, there is many (I believe) software projects, who want to be prepared to RISC-V architecture and they start experimenting with these new instructions. Obviously, using compiler intrinsics is much easier then straight ASM coding. I do not think that evolution of intrinsics syntax during it being developed is a big issue. I just think providing kind of "compatibility mapper" in header file (something like #define OLD_INTRINSIC_1 NEW_INTRINSIC_1 (if that is possible) would be helpful, as software development might continue while commette working on final version of intrinsics

eopXD commented 1 year ago

Hi @mshabunin and @vladimir-dudnik-1,

Main changes from v0.11 to v0.12 are

  1. Introduce tuple type for use of segment load / store intrinsics and remove the existing ones that passes multiple RVV type pointers as function parameters. (#198)
  2. Introduce "explicit-frm" intrinsics that allows floating-point rounding mode (frm) control for floating-point intrinsics (#226)
  3. Introduce fixed-point intrinsics with fixed-point rounding mode (vxrm) control and remove the existing intrinsics without the rounding mode parameter. (#222)

Incompatible changes are (1) and (3).

I am worried of supporting compatibility for (3), because our intention for removing them is to have users be aware of their rounding mode when calling fixed-point intrinsics. With us removing vread_csr and vwrite_csr already, we do not hope the users to think that the legacy fixed-point intrinsics can be used in the future.

The specification does not define a default rounding mode for the fixed-point instructions, which also makes supporting compatibility pretty awkward. In my opinion, supporting compatibility for legacy fixed-point intrinsics is to model them as dynamic, and use what is already in the vxrm CSR. This cannot be done with a compatible header using #define, but rather a patch into the compiler.

In the past meeting, I mentioned that I can provide a patch that can be applied to the upstream LLVM compiler for compatibility support. Going over this thought again, I don't think it is a good idea because I will have to continue to maintain the patch to be applied without conflict, and the longer we allow the compatibility of such, the more codebase will be grown upon legacy interfaces.

If you have any questions regarding the transition, please do message me and I can help going through the pull requests and give help as much as possible.