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

Unify intrinsic interface for read/write vCSRs #204

Closed
wants to merge 1 commit into from

Conversation

rjiejie
Copy link
Contributor

@rjiejie rjiejie commented Feb 28, 2023

For C lang macro implementation, it will bring some incompatibility,
we should consider to unify the interface of CSRs, like format as other RVV intrinsics https://github.com/riscv-non-isa/riscv-c-api-doc/blob/master/riscv-c-api.md#intrinsic-functions
__riscv_read/write_csr_[REGISTER NAME]

@rjiejie
Copy link
Contributor Author

rjiejie commented Feb 28, 2023

@cmuellner @kito-cheng
Any comments or suggestions ?

@cmuellner
Copy link
Contributor

A unified CSR read/write intrinsic is also part of the goals for the upcoming intrinsics TG.

See also https://docs.google.com/document/d/1wQebF9S8LKblxCWKv2XGSWiQCMhLibphMBKhCovi7vk/edit

@eopXD
Copy link
Collaborator

eopXD commented Mar 1, 2023

Considering that we will have rounding mode intrinsics (to model vxrm and frm) for fixed-point and floating-point, and also have intrinsics for the exception flags (vxsat and fflag), plus that vstart is generally kept as zero , I think we should give a second thought on having these interface for the RVV intrinsics for the long future. That was the reason why didn't extend the exposure of vlenb upon the vread_csr functions in #182.

@rjiejie
Copy link
Contributor Author

rjiejie commented Mar 3, 2023

@eopXD @cmuellner Ok, looks good :)
Hope that we can ratify specification soon, the macro implementation is not good choice.

@eopXD
Copy link
Collaborator

eopXD commented Apr 28, 2023

If there is no further concern, please allow me to close this PR.


unsigned long vread_csr(enum RVV_CSR csr);
void vwrite_csr(enum RVV_CSR csr, unsigned long value);
unsigned long __riscv_read_csr_vxsat();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is unsigned long the right type for a XLEN-bit value?

I.e. the combination of RV64 with ILP32 could cause issues.

We probably want intX_t and uintX_t for RISC-V.

@kito-cheng: any ideas?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that should be intxlen_t would be more semantic correct here, let push this riscv-non-isa/riscv-c-api-doc#14 again next week.

Choose a reason for hiding this comment

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

A couple notes:

  • One doesn't need a type exactly sized as the CSR, but only long enough (e.g., intmax_t would always work).
  • Types that start with 'int' may end up being used in future standardization of inttypes.h, so we should stay out of that namespace.
  • We already have the __riscv_xlen macro. So using the preprocessor, one can always determine the actual number of bytes that a CSR is wide.
  • What about __riscv_flen? Would you want to introduce another type for 'an integer that is as wide as the longest floating point register'?

Pondering all of the above points, I would recommend against introducing such a new type — all we need is a type that is at least as wide as a CSR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi all,
I see that there is already on-going discussion in the SIG toolchain mailing list [0], please proceed there as I have responded, we plan to remove vread_csr and vwrite_csr, and the RVV type discussion should be hosted there.

[0] https://lists.riscv.org/g/sig-toolchains/message/599

@eopXD eopXD closed this May 3, 2023
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.

5 participants