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

RVV compile option to enable decoding vector instructions #357

Merged
merged 3 commits into from
Jan 11, 2025

Conversation

kpgriesser
Copy link
Collaborator

This PR adds a compile option, RVV, to allow REV to be built to support the vector coprocessor. This PR is required for a forthcoming PR in the R2V2 repository that decouples that design from the REV source tree.

@leekillough
Copy link
Collaborator

We should compare / consolidate this with the REVVEC macro definition.

#ifdef REVVEC
#include "insns/RVVec.h"
#endif

…ow resides in the vector coprocessor source tree
@kpgriesser
Copy link
Collaborator Author

kpgriesser commented Jan 10, 2025

RVVec.h,. the vector instruction implementations, no longer exists in the rev source tree. The now independent coprocessor now includes this header so we can remove the #ifdef REVVEC code block. ( Tested with 'solo' branch in r2v2 repo )

@leekillough
Copy link
Collaborator

leekillough commented Jan 10, 2025

I was mainly speaking of the REVVEC vs RVV naming discrepancy -- consolidate one and retire the other, as well as integrate them into the build. grep REVVEC src/* include/* include/insns/* to make sure there aren't others.

I don't think we need the RevFeature V extension parsing to be enabled/disabled by an #ifdef. Like Zicntr, a runtime error can be printed if a V coprocessor is not available.

@kpgriesser
Copy link
Collaborator Author

There are no other REVVEC defines. If nobody has an issue with have the V extension parsing on by default ( I don't ) then we can just remove the RVV define as well and not require any special compile.

@leekillough
Copy link
Collaborator

I would prefer if we didn't have to build Rev differently. I think the parsing of the V extension can always be supported. If the V instructions are handled by the coprocessor during decode, and the coprocessor code has access to RevFeature (directly or indirectly -- we can always add more accessors if the current passkey mechanism is insufficient), then it can tell whether to decode V instructions or just ignore them and cause a normal unable to decode instruction error in Rev Core. Or if it wants to be more fancy, the coprocessor can decode V instructions as if the V extension is enabled, and then abort with a runtime error if the V feature is not really enabled, similar to how Zicntr works today.

@kpgriesser
Copy link
Collaborator Author

I agree. My last push removes the RVV define. So this PR is essentially just removing REVVEC. From every other users' standpoint it is identical to the devel branch.

@kpgriesser kpgriesser merged commit bd49d1d into devel Jan 11, 2025
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.

2 participants