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 riscv_int.h and define int_xlen_t and uint_xlen_t #14

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kito-cheng
Copy link
Collaborator

@kito-cheng kito-cheng commented Dec 9, 2020

This PR is the product of this RFC[1], the main purpose is provide a pair of integer type with XLEN-bits.

According the discussion, many people are concern about we should not add the type directly, that will corrupt the global namespace, so it would be great if we put it in the header files.

So why it put in riscv_intrinsic.h? here is two reason, first reason is the initial request are come from intrinsic function interface, so put they together should be reasonable, second reason is I have no idea which file name is the best :P riscv.h is little bit too generic and sounds like easy to conflict with exist headers - as I know at least gcc, binutils and gdb has one.

[1] riscv-non-isa/riscv-elf-psabi-doc#158

@jim-wilson
Copy link
Collaborator

The B extension proposal calls for a rvintrin.h file.
https://github.com/riscv/riscv-bitmanip/blob/master/texsrc/bext.tex#L2403
https://github.com/riscv/riscv-bitmanip/blob/master/cproofs/rvintrin.h
I don't care about the file name. Either rvintrin.h or riscv_intrinsic.h is OK with me. Or anything else is reasonable. Looking at gcc ports, I see a lot more examples of the *intrin.h pattern than the *intrinsic.h pattern.

The B extension is proposing that there is that we have a single intrinsic header file for riscv, though given the size of the V extension, I'm not sure if it is a good idea to force people to include the V support when they don't want it. Maybe put each extension in its own file, and then include it in the master (rvintrin.h?) file, maybe only if the appropriate extension is enabled.

I don't care about the type names. People have asked for this feature before, so adding the types is useful.

It isn't obvious what the MIN MAX macros are for, or how the types can hvae min/max sizes.

@nick-knight
Copy link

nick-knight commented Dec 10, 2020

@jim-wilson I think the macros are meant to be the natural extensions of ones in limits.h and stdint.h.

I wonder if it's worth specifying that int_xlen_t is two's complement, and both int_xlen_t and uint_xlen_t have no padding bits (cf. stdint.h). I've made these suggestions below.

riscv-c-api.md Outdated Show resolved Hide resolved
riscv-c-api.md Outdated Show resolved Hide resolved
riscv-c-api.md Outdated Show resolved Hide resolved
@jim-wilson
Copy link
Collaborator

My brain was stuck on type sizes, which don't have min and mix, but min and max representable values makes perfect sense.

@nick-knight
Copy link

nick-knight commented Dec 10, 2020

While we're at it, there's also the U?INT_XLEN_C, PRI[diuoxX]XLEN, and SCN[diuox]XLEN macros.
https://en.cppreference.com/w/c/types/integer
Happy to add these in a separate PR.

@kito-cheng
Copy link
Collaborator Author

The B extension is proposing that there is that we have a single intrinsic header file for riscv, though given the size of the V extension, I'm not sure if it is a good idea to force people to include the V support when they don't want it. Maybe put each extension in its own file, and then include it in the master (rvintrin.h?) file, maybe only if the appropriate extension is enabled.

Oh, maybe my word not clear enough, but I intend to do that, e.g. only included V-ext intrinsic when V or zv* extensions is enabled.

+ This file is universal intrinsic header file for all extensions, each extension
+ could have their own header files, but should included in this file if
+ corresponding extension is enabled.

@kito-cheng
Copy link
Collaborator Author

While we're at it, there's also the U?INT_XLEN_C, PRI[diuoxX]XLEN, and SCN[diuox]XLEN macros.
https://en.cppreference.com/w/c/types/integer
Happy to add these in a separate PR.

@knightsifive thanks, in fact I was refer to the same page before :P

Copy link

@nick-knight nick-knight left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@cmuellner
Copy link
Collaborator

I think there are two issues to resolve/decide to land this:

  • gating the types behind a header (and if so, which header file name)
  • chose an alternative name to avoid namespace collision or at least namespace pollution

I want to avoid these types being gated by a header file with the name "intrinsic" in it. Intrinsics are just some use cases. So I am for either nothing or __riscv.h (or similar).

If namespace pollution is a concern, then I am also ok with a name like __riscv_int_xlen_t and __riscv_uint_xlen_t.

I also agree about creating a complete set of support macros. However, we should drop UINT_XLEN_MIN from the PR.

@kito-cheng kito-cheng changed the title Add riscv_intrinsic.h and define int_xlen_t and uint_xlen_t Add riscv_int.h and define int_xlen_t and uint_xlen_t Jul 7, 2023
@kito-cheng
Copy link
Collaborator Author

I just narrow down the scope of this PR, and made this specific to adding xlen-integer type only, also add print/scan modifier macros.

Also rename the file to riscv_int.h and adding prefix __RISCV_/__riscv_.

@kito-cheng
Copy link
Collaborator Author

@cmuellner
Copy link
Collaborator

@kito-cheng, do you have a particular use case for these types?

Last time we discussed this (I raised the topic in the Toolchain SIG call), we only identified shared code for RV32/RV64 that accesses CSRs as a potential candidate. But that could be solved differently as well and would not really justify the introduction of a new type. Therefore, we deferred this topic back then.

@kito-cheng
Copy link
Collaborator Author

@cmuellner I guess I didn't give the context why I restart that again, the reason I restart is because @topperc is working on scalar intrinsic like #44, we discuss some of intrinsic other than scalar crypto might need xlen type, but long is not really suitable for that since ilp32/rv64 seems not so far (okay, still long road, but not far as before :P), so I think it good timing to restart and also including your suggestion before (I was really dislike __riscv_ prefix but those potential issue is more important than personal preference/taste).

LLVM implementation: https://reviews.llvm.org/D154706

riscv-c-api.md Outdated Show resolved Hide resolved
riscv-c-api.md Outdated Show resolved Hide resolved
@kito-cheng
Copy link
Collaborator Author

Changes:

  • Fix typos

@topperc
Copy link
Contributor

topperc commented Jul 10, 2023

I'm not convinced of the need for this. I don't see a strong use case of writing xlen aware bitmanipulation code. Some crypto algorithms need to be different for RV32 and RV64, but once you've done that you know what xlen is and can use the 32/64 bitmanip intrinsics directly as needed.

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