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 0.11 -> 0.12 compatibility headers #255

Open
mshabunin opened this issue Aug 9, 2023 · 11 comments
Open

Add 0.11 -> 0.12 compatibility headers #255

mshabunin opened this issue Aug 9, 2023 · 11 comments

Comments

@mshabunin
Copy link

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
Copy link

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
Copy link
Collaborator

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
Copy link
Collaborator

asb commented Sep 4, 2023

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
Copy link
Collaborator

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
Copy link

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

@nick-knight
Copy link
Collaborator

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

@sunshaoce
Copy link

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
Copy link
Collaborator

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
Copy link
Author

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:

  • large part of this repository is auto-generated, so could be the compatibility headers
  • having single official source might be useful for many projects

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
Copy link

@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
Copy link
Collaborator

eopXD commented Sep 6, 2023

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. (Improve segment load/store interface with tuple types #198)
  2. Introduce "explicit-frm" intrinsics that allows floating-point rounding mode (frm) control for floating-point intrinsics (Model rounding mode 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. (Model rounding mode control for fixed-point intrinsics #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants