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

Clean up RVV register composition. #521

Merged
merged 2 commits into from
Mar 7, 2024

Conversation

sh1boot
Copy link
Contributor

@sh1boot sh1boot commented Feb 16, 2024

Prefer use of _riscv_vcreate*() intrinsics, where appropriate. This
does away with piecemeal assembly of a tuple register types and ensures
that every component is defined. It also gets rid of uninitialised
variable warnings.

__riscv_vcreate intrinsics aren't available in older toolchains but will
be available in version 1.0 of the intrinsics interface. In the interim
include substitute macros which try to fill the gap as best as possible.

Use a regular pattern of cast-then-insert, and extract-then-cast when
dealing with tuple registers with mixed component types.

Remove a few unused macros, etc..

@sh1boot
Copy link
Contributor Author

sh1boot commented Feb 16, 2024

This contains a portion of #503, and fits underneath that PR and on top of #520, so that each can be reviewed in isolation.

Diff from #520: rivosinc/sleef@dev/shosie/fix-rvv-masks...dev/shosie/fix-rvv-warnings

src/arch/helperrvv.h Outdated Show resolved Hide resolved
src/arch/helperrvv.h Show resolved Hide resolved
src/arch/helperrvv.h Outdated Show resolved Hide resolved
@sh1boot sh1boot marked this pull request as ready for review February 24, 2024 00:22
// corresponding ENABLE_RVV_SP or ENABLE_RVV_DP macro guards.
#if defined(ENABLE_RVV_SP) && defined(ENABLE_RVV_DP)
#error Cannot simultaneously define ENABLE_RVV_SP and ENABLE_RVV_DP
#define SLEEF_RVV_UNINITIALIZED_REG(T) __extension__({ \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please comment and explain a bit what is going on here? We get a sense that this is GCC dependent, but still seems to build correctly with llvm. Also what does it do?

Copy link
Contributor Author

@sh1boot sh1boot Mar 1, 2024

Choose a reason for hiding this comment

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

The extension used here is supported by GCC and Clang, but perhaps it may be in issue for other compilers.

If you recall that (a, b, c) evaluates a, b, and c, and returns c, this extension with {} inside of the () allows more complex logic and the creation of temporary variables, and the last whole statement is the result (the pragma doesn't count). In this instance I just want to create a temporary variable that is undefined, and return it; because the chain of __riscv_vset() operations will fill every part of the final result.

The __extension__ keyword just clears this with the linter saying that I have not forgotten that it doesn't conform to standard C (also supported in both GCC and Clang).

But I did forget that I went outside of standard C, to be honest. It was hammered out in anger when I discovered the version incompatibility.

Before I go and write something to that effect in comments, are we OK with use of a GCC/Clang extension which might interfere with other compilers? They can avoid this problem just as soon as they adopt the 1.0 version of the intrinsics specification and this workaround won't be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking back at how it's used; I made the macro expecting to have to use it many more times than I eventually did.

Since there are only two of them, it's simpler to just use a couple of inline functions.

Sadly the pragmas are stripped by the preprocessor for inline headers (both #pragma and _Pragma are stripped), so when it comes to generating inline headers the warnings will come back. When the toolchain is updated this nit also goes away.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Before I go and write something to that effect in comments, are we OK with use of a GCC/Clang extension which might interfere with other compilers?

I think this is an acceptable limitation since the support for other compilers retrograded to experimental. Is it still the case with the inline functions?

Sadly the pragmas are stripped by the preprocessor for inline headers (both #pragma and _Pragma are stripped), so when it comes to generating inline headers the warnings will come back. When the toolchain is updated this nit also goes away.

Does it not help to add //@#pragma ...? I suppose we can deal with a few warnings until the toolchain is updated.

If you find a way to avoid that altogether in the future that's obviously preferable, but for now I'm happy with the most recent change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tried it some //@ tricks, but it looks like the preprocessor eats up the pragmas as soon as they appear, leaving no opportunity to squeeze them through to the next layer.

There might be something more esoteric with token pasting and/or conditional code that recognises that the file is being preprocessed, but if I ever figure that out I think it would be better to do it in a separate tidy-up patch.

So as things stand this is the best I know how to do.

Having replaced the macros with inline functions they should now be portable to any compiler again. And the rules for pragmas is that if a compiler doesn't know it it just ignores it (hence GCC putting their own name at the front to isolate everything under that to their own namespace so there are no collisions).

I think this should be good to go, now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, looks good to me

@blapie
Copy link
Collaborator

blapie commented Mar 6, 2024

It looks like there are some conflicts? Did I mess it up by merging #520 first, or do you need to rebase your PR after each merge?

Simon Hosie added 2 commits March 6, 2024 09:49
Prefer use of __riscv_vcreate_*() intrinsics, where appropriate.  This
does away with piecemeal assembly of a tuple register types and ensures
that every component is defined.  It also gets rid of uninitialised
variable warnings.

__riscv_vcreate intrinsics aren't available in older toolchains but will
be available in version 1.0 of the intrinsics interface.  In the interim
include substitute macros which try to fill the gap as best as possible.

Use a regular pattern of cast-then-insert, and extract-then-cast when
dealing with tuple registers with mixed component types.

Remove a few unused macros, etc..
@sh1boot sh1boot force-pushed the dev/shosie/fix-rvv-warnings branch from fb4270e to 1314848 Compare March 6, 2024 17:50
@sh1boot
Copy link
Contributor Author

sh1boot commented Mar 6, 2024

I'm not very clear on the best flow with staging multiple dependent PRs on github. I've rebased this change, and I guess I'll rebase the others once this merges, too.

@blapie blapie merged commit 3896b07 into shibatch:master Mar 7, 2024
31 checks passed
@luhenry luhenry deleted the dev/shosie/fix-rvv-warnings branch March 25, 2024 08:00
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.

3 participants