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

How to support backward incompatible API. #145

Closed
zakk0610 opened this issue Mar 22, 2022 · 5 comments
Closed

How to support backward incompatible API. #145

zakk0610 opened this issue Mar 22, 2022 · 5 comments
Labels
Resolve for v1.0 Feature or problems we will close before the v1.0 release

Comments

@zakk0610
Copy link
Collaborator

zakk0610 commented Mar 22, 2022

We have several proposals which are trying to change the current API (ex. renaming or reorder the argument order), and some of them make the design more consistent and reasonable.

For examples:

  1. Move mask argument positions to first for merge intrinsic function. #140
  2. [RFC] Support a new set of policy intrinsic functions. #137 (comment) (Remove masked-off parameters for masked segment loads)

These changes aren't backward compatible. If we want to make the design better and adapt these changes, we would face backward incompatibility. If we agree that we want to do this, then we need to have consensus on how to support that.

I have two proposals as shown below:

  1. Compiler supports pre-defined macro for RVV API version.
    The Compiler could optionally support different versions of RVV API based on tags in rvv-intrinsic-doc. For now we have v0.10 and v0.9 tag. We can have a predefined macro __riscv_v_intrinsic_version to imply the supported version information which follows the formula we already have in https://github.com/riscv-non-isa/riscv-c-api-doc/blob/master/riscv-c-api.md#architecture-extension-test-macro (<MAJOR_VERSION> * 1,000,000 + <MINOR_VERSION> * 1,000 + <REVISION_VERSION>)

We will have the opportunity to update API because we have now versioned them.

// user's code
#if __riscv_v_intrinsic_version == 10000
// v0.10 api
#elif __riscv_v_intrinsic_version == 9000
// v0.9 api
#else
#error unsupported rvv intrinsic functions version.
#endif

or

// user's code
#define __riscv_v_intrinsic_version 9000
#include <riscv_vector.h>

// v0.9 api


  1. We directly support the new set of intrinsic functions in another namespace, which the function naming would follow the rule in RFC: Intrinsics: Define common availability and naming rules riscv-c-api-doc#25, to add __rvv_ prefix in front of the intrinsics, and stored in a separate intrinsic header (with the header name following the rule proposed in Uniform intrinsic filename riscv-c-api-doc#17).

For example, new naming will look like vint8m1_t __rvv_vadd_vv_i8m1(vint8m1_t vs2, vint8m1_t vs1, size_t vl); under header file name like riscv_v_intrinsic.h.

In this way, we also could do anything we want in the new intrinsic API set.


This is maybe our last time to update the naming (maybe overloading version only), argument number and order, so we need to have comprehensive consideration and review all proposals in the issue list.

Both proposals may require a schedule for API freeze day, and we also need to list what are missing now..

@zakk0610 zakk0610 changed the title How to support backward incompatibilities API. How to support backward incompatible API. Mar 22, 2022
@eopXD
Copy link
Collaborator

eopXD commented Mar 22, 2022

I think this might be the last chance to redesign RVV C intrinsic API as the v-spec is now ratified. So I guess we can have one last big remake before the API settles. I think 2. is better as it would unify with other extensions' intrinsic. Also I think it would be more intuitive if the new set of backward incompatible intrinsic are just named differently.

We can allow the two headers to coexist and deprecate them in the future.

@zhongjuzhe
Copy link

zhongjuzhe commented Mar 23, 2022

The RVV GCC is in the RISC-V foundation repo but not push to GCC upstream yet. I don't think there is backward compatible issue in GCC because I can push them upstream after you freeze the document. Do you want me to support the old obsolete intrinsics before pushing them upstream? I am Ok with that. It I need to do so, Can you list the api both 0.9 version and 1.0 version together in the document?
For example:
version 0.9:
vmerge_vv_i8m1 (vint8m1_t v0, vint8m1_t v1, vbool8_t mask, size_t vl)
version 1.0:
vmerge_vv_i8m1 (vbool8_t mask, vint8m1_t v0, vint8m1_t v1, size_t vl)

If the api are the same both 0.9 and 1.0 version. Just keep the document no need to change.
Besides, I am waiting for the RVV intrinsic document to be frozen.

@eopXD
Copy link
Collaborator

eopXD commented Aug 2, 2022

I think we may not want to break backward compatibility in any sense. We may improve the user experience by extending the intrinsic API.

Maybe we can discuss on how extensions can be added to the intrinsic after this v1.0 release?

@eopXD eopXD added the Revisit after v1.0 Features or problems we will revisit after the v1.0 release label Aug 2, 2022
@eopXD
Copy link
Collaborator

eopXD commented Sep 8, 2022

I want to raise the discussion regarding backward compatibility back again. I originally thought that maybe we should defer the problem of fixing these glitches to discussion after v1.0, however after a second thought, maybe v1.0 is the best timing to apply incompatible changes since we have not reached a "formal" release yet (nothing is ratified).

I think that changes that is backed with functional or compiler optimization reason justifies themself.

On the other hand, people may find some glitches in the current intrinsic API-s. For example, issue #159 pointed out that there is inconsistency on some functions where it can be expressed in reduce form without ambiguous naming but has output type appended in the function name. Another example is #140, where the argument orderings are inconsistent.

Taking this further, we may always come back to the same dilemma, where we consider software stability on one side and improving the API on the other. So I think it is important we should take the courage to talk about this and rule out some SOP that we can follow.

Assuming that an incompatible change have gathered approval from the community, how do we apply it?

A straightforward approach is that the approved incompatible change be applied in the next release. A more conservative approach is we find a way to have deprecation warnings of the change in the next version, and have the change be applied in the next-next version.

I personally think we should take a more conservative approach on this if we decide to formalize an approach to resolve modifications of incompatible change. Please share your thoughts on this.

@eopXD eopXD added Discussion wanted Resolve for v1.0 Feature or problems we will close before the v1.0 release and removed Revisit after v1.0 Features or problems we will revisit after the v1.0 release labels Sep 8, 2022
@rjiejie
Copy link
Contributor

rjiejie commented Oct 27, 2022

This issue like Vector ISA, maybe for all ISA version issue,
we need to considerate multi-versions incompatibility.

+1 for separated specific version intrinsic header like as riscv_v_intrinsic_<version>.h
e.g. riscv_v_intrinsic_v0p7.h

@eopXD eopXD closed this as completed Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolve for v1.0 Feature or problems we will close before the v1.0 release
Projects
None yet
Development

No branches or pull requests

4 participants