Skip to content

Conversation

@preames
Copy link
Contributor

@preames preames commented Jan 24, 2025

This change reserves the vendor prefix "ri." for Rivos's custom extensions.

This change reserves the vendor prefix "rv." for Rivos's custom extensions.  

Signed-off-by: Philip Reames <[email protected]>
@TommyMurphyTM1234
Copy link

Might "rv" perhaps get confused with official "RISC-V" standard extensions?

@preames
Copy link
Contributor Author

preames commented Jan 24, 2025

Might "rv" perhaps get confused with official "RISC-V" standard extensions?

The mnemonics for standard extensions are not prefixed.

@TommyMurphyTM1234
Copy link

Might "rv" perhaps get confused with official "RISC-V" standard extensions?

The mnemonics for standard extensions are not prefixed.

I appreciate that, but isn't it possible that some casual observers won't know that and might mistakenly assume that "rv" prefixed extensions might be official/standard/non-proprietary RISC-V? Especially since "RV" is so widely used as an informal way to refer to RISC-V features in general. An easy way to eliminate the possibility of such confusion might be by using a different prefix here?

@cmuellner
Copy link
Collaborator

We talked about this in the SIG Toolchain call today.

I acknowledge the potential for confusion with an rv. prefix.
However, vendor code is not expected to be emitted without explicitly enabling it (similar arguments apply to other occurrences of vendor instructions like hand-written assembly).

Where it might cause confusion is in the intrinsic names.
There, we have a prefix of __riscv_ followed by the vendor prefix.
With accepting rv, we could get intrinsics like __riscv_rv_vadd_vv_i8m1() (similar to __riscv_vadd_vv_i8m1).

I stated that I wouldn't block this PR, but I'd like to hear opinions from GCC and LLVM maintainers on this.

@cmuellner
Copy link
Collaborator

The consensus in the RISC-V LLVM sync-up call (lead by @asb) was to accept this.

@kito-cheng, any opinion here?

@kito-cheng
Copy link
Collaborator

I acknowledge the potential for confusion with an rv. prefix. However, vendor code is not expected to be emitted without explicitly enabling it (similar arguments apply to other occurrences of vendor instructions like hand-written assembly).

Where it might cause confusion is in the intrinsic names. There, we have a prefix of __riscv_ followed by the vendor prefix. With accepting rv, we could get intrinsics like __riscv_rv_vadd_vv_i8m1() (similar to __riscv_vadd_vv_i8m1).

I stated that I wouldn't block this PR, but I'd like to hear opinions from GCC and LLVM maintainers on this.

I was also hesitate about that may confused by riscv as well, but that sound reasonable with those two reasons.

@kito-cheng
Copy link
Collaborator

So yeah, LGTM :)

@preames
Copy link
Contributor Author

preames commented Feb 25, 2025

FYI, triggered by the discussion here and in the LLVM reviews, we have changed the specification for the instructions (https://github.com/rivosinc/rivos-custom-extensions) to use the "ri." prefix instead of "rv." To date, only experimental support has landed in LLVM and we just changed that over as well.

@kito-cheng If you're good with the change, could you land the new version?

@cmuellner
Copy link
Collaborator

Related LLVM PR: llvm/llvm-project#128761

@cmuellner
Copy link
Collaborator

LLVM merged the ri vendor prefix for Rivos.
So, let's land this as well.

@cmuellner cmuellner merged commit 1b656e3 into riscv-non-isa:main Feb 27, 2025
2 checks passed
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.

4 participants