Skip to content

Conversation

@djtodoro
Copy link
Contributor

@djtodoro djtodoro commented Dec 31, 2024

Adding extensions for MIPS vendor:
1. xmipscmov
2. xmipslsp
3. xmipscbop
4. xmipsexectl

The official product page here:
https://mips.com/products/hardware/p8700/

Upstreaming of this in LLVM and binutils is in progress.

@djtodoro
Copy link
Contributor Author

cc @asb :)

@lenary
Copy link
Contributor

lenary commented Jan 2, 2025

This looks good to me, and uncontroversial :)

@cmuellner
Copy link
Collaborator

Thanks for the PR!

I see that the references to the two new extensions go to the product description of the MIPS P8700 Series.
If that product retires, will the extensions also retire? If not (which I assume), is there a reference for a description of the extensions independent of the product they implement?

Also, the docs on the product page don't mention the two extensions (Xmipscmov and Xmipslsp). The appendix of the P8700 programmer's guide only lists nine custom instructions (ccmov, ehb, ihb, ldp, lwp, mtlbwr, pref, sdp, swp). This repo is not the right place to define extensions. Instead, a document should be referenced where the extensions are defined (incl. CSRs, instructions, etc).

A specification of a custom extension should also include the vendor prefix (e.g. mips.ccmov).

@djtodoro
Copy link
Contributor Author

Thanks for the PR!

I see that the references to the two new extensions go to the product description of the MIPS P8700 Series. If that product retires, will the extensions also retire? If not (which I assume), is there a reference for a description of the extensions independent of the product they implement?

Also, the docs on the product page don't mention the two extensions (Xmipscmov and Xmipslsp). The appendix of the P8700 programmer's guide only lists nine custom instructions (ccmov, ehb, ihb, ldp, lwp, mtlbwr, pref, sdp, swp). This repo is not the right place to define extensions. Instead, a document should be referenced where the extensions are defined (incl. CSRs, instructions, etc).

A specification of a custom extension should also include the vendor prefix (e.g. mips.ccmov).

@cmuellner thanks for the comment! Sorry for the delay here. We have updated the document at [0], and in the MIPS Defined Instructions section, you can see that we have added the mips.* prefix to all of our extensions.

https://mips.com/wp-content/uploads/2025/03/P8700-F_Programmers_Reference_Manual_Rev1.81_3-3-2025.pdf

@djtodoro
Copy link
Contributor Author

Do you have any comment on this?

@apazos
Copy link
Contributor

apazos commented Mar 26, 2025

LGTM, @cmuellner any final comments?

@djtodoro
Copy link
Contributor Author

djtodoro commented Apr 2, 2025

Ping. :) Any comments?

@djtodoro
Copy link
Contributor Author

djtodoro commented Apr 9, 2025

Ping. :)

Copy link
Collaborator

@kito-cheng kito-cheng left a comment

Choose a reason for hiding this comment

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

LGTM as a RISC-V GCC maintainer

@cmuellner
Copy link
Collaborator

The links are already out-of-date. On the referenced page are "Product Brief", "Programmer's Guide", and "Data Sheet". However, the PR states "MIPS p8700 specification", which is not on the linked page.
I assume that the "Programmer's Guide" is the relevant document.
So, I suggest changing the text in the brackets to "MIPS P8700 Programmer's Guide".

Another thing that I noticed is that the PR introduces an extension with the name "xmipscmove," but the document names it "xmipscmov" (without the e at the end).

If these two items are addressed, we can land this.

@djtodoro
Copy link
Contributor Author

@cmuellner thanks for the comment. I have addressed it and updated the links. Thank you.

Adding extensions for MIPS vendor:

1. xmipscmov
2. xmipslsp
3. xmipscbop
4. xmipsexectl

The official product page here:
https://mips.com/products/hardware/p8700/
Copy link
Collaborator

@cmuellner cmuellner left a comment

Choose a reason for hiding this comment

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

LGTM

@cmuellner cmuellner merged commit e45b576 into riscv-non-isa:main Apr 14, 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.

5 participants