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

Use wider integers for shifts #68

Closed
nick-knight opened this issue Feb 24, 2021 · 5 comments
Closed

Use wider integers for shifts #68

nick-knight opened this issue Feb 24, 2021 · 5 comments

Comments

@nick-knight
Copy link
Collaborator

nick-knight commented Feb 24, 2021

I'm concerned about several intrinsics which input a "shift" amount as uint8_t. This only allows shifts up to 255, which fails to expose underlying functionality for larger SEW (and ELEN).

Particular examples are vsll_vx, vsr{a,l}_vx, vnsr{a,l}_vx, vssr{a,l}_vx, and vnclip{,u}_wx. I only skimmed quickly; perhaps there are more.

The vv/wv forms use an appropriate type-width --- one that increases with SEW --- for their (vector) op2, and I suggest matching that in the vx/wx forms.

(It's true that the vi forms are restricted to a 5-bit op2, but these forms aren't exposed in the intrinsics API.)

EDIT: on further thought, perhaps it's more appropriate to use uintXLEN_t (or whatever it's called).

@zakk0610
Copy link
Collaborator

Agree, spec already reserves SEW=1024 supported.

Unfortunately uintXLEN_t is still in the PR, riscv-non-isa/riscv-c-api-doc#14
Which type are you prefer to use? unsigned long?

@nick-knight
Copy link
Collaborator Author

I propose we just define uint_xlen_t in the usual way in the riscv_vector.h, and delete it when the C API PR is approved and added to compilers. Hopefully this can be done without breaking user code because riscv_vector.h is bundled with the toolchain.

I'm pretty sure the P-extension does the same thing in their intrinsics header file. I don't know about other extensions' intrinsics.

I'm not an expert on this kind of thing, happy to hear alternatives.

@kito-cheng
Copy link
Collaborator

I propose we just define uint_xlen_t in the usual way in the riscv_vector.h, and delete it when the C API PR is approved and added to compilers. Hopefully this can be done without breaking user code because riscv_vector.h is bundled with the toolchain.

That SGTM, I think intrinsic for B extension should need that too.

@zakk0610
Copy link
Collaborator

zakk0610 commented Mar 11, 2021

Before we have uint_xlen_t, could we use size_t type for shift amount ?

@nick-knight
Copy link
Collaborator Author

This was addressed by #72 , although I think this and related aspects of the API should be revisited once we have XLEN-width integer types to replace size_t, long, etc.

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

No branches or pull requests

3 participants