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

Update Instructions with riscv-opcodes #2972

Merged
merged 4 commits into from
May 6, 2022

Conversation

ZenithalHourlyRate
Copy link
Contributor

Related issue: riscv/riscv-opcodes#106

Type of change: other enhancement

Impact: API modification

Development Phase: proposal (waiting on riscv/riscv-opcodes#111)

There is tmply a workaround for CI (thus this PR is a draft now). Will be force pushed once the issue is resolved.

Release Notes

  • Updated Instructions from riscv-opcodes
  • Fixed legacy instruction name
  • Separate RoCC instructions into CustomInstructions.scala

@ZenithalHourlyRate
Copy link
Contributor Author

ZenithalHourlyRate commented May 4, 2022

Before the new riscv-opcodes, we rely on SLLI_RV32 to make both RV32 and RV64 work in one Instructions.scala, however, in the new version we do not have such convenience as the API has changed.

One more bug to note here is that we currently do not have BINVI_RV32 (BINVI has different encoding for RV32 and RV64) so the current Instructions.scala in master does not work for RV32 Zbs extension (thus #2950 is affected). This actually is a bug from old riscv-opcodes (i.e. not generating BINVI_RV32).

So to use the new riscv-opcode and fix the BINVI_RV32 issue, we need two file Instructions64.scala (generated by make EXTENSIONS="rv_* rv64*" inst.chisel and rename object Instructions to object Instructions64) and Instructions32.scala (generated by make EXTENSIONS="rv32*" inst.chisel and rename to object Instructions32), and for IDecode.scala, it would import Instructions64._ globally and for class I32Decode it would use Instructions32.SLLI instead of SLLI_RV32.

@sequencer sequencer merged commit 850e1d5 into chipsalliance:master May 6, 2022
Copy link
Member

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

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

I am just amused that, over the course of 42 minutes, we went from "I can not decide on this. This needs @aswaterman" to "click the merge button". 😆

@sequencer
Copy link
Member

sorry @aswaterman I mean this PR
seems to be ok.(didn’t break any thing, but just bump the Instruction.scala.
The issue is about the removing custom instructions, I think this need your opinion to decide to keep as it is or we can remove it since it wont be generated from riscv-opcodes anymore.

@aswaterman
Copy link
Member

I agree, it is fine; I just thought it was funny.

@sequencer
Copy link
Member

Thanks for understanding, I just wanna speed up the developing procedure. If some PR do have some issues or you don't like it, we can at least revert them.

@ZenithalHourlyRate ZenithalHourlyRate deleted the new-opcodes branch May 6, 2022 11:45
@michael-etzkorn
Copy link
Contributor

michael-etzkorn commented May 6, 2022

Thanks for understanding, I just wanna speed up the developing procedure. If some PR do have some issues or you don't like it, we can at least revert them.

Ok with the speed of PRs so long as we start a versioning scheme (doesn't need to match how software usually does it) and a changelog that regularly keeps up. The git commit history is great and all, but if main heads in the direction of being more of a developer branch, we should definitely tag the most stable commits and document the PR changes. I'll lead charge on this starting with the last version tag up to the Chisel 3.5 bump PR.

@sequencer
Copy link
Member

sequencer commented May 6, 2022

Ok with the speed of PRs so long as we start a versioning scheme (doesn't need to match how software usually does it) and a changelog that regularly keeps up.

We discussed in the working group meeting:

  1. Currently we won't guarantee the master being API stable, since all APIs in RC is public.
  2. The only versioning scheme currently is the commit-based version.
  3. There won't be any bug fix to backport to a specific "stable" version.

we should definitely tag the most stable commits and document the PR changes.

How do we define the "most stable commits"?

For any companies/users, they are free to stick at any commits, or fork a branch to provide their own "stable" version, but I believe the RCW won't guarantee a specific commit/branch is "stable" enough for any usage.
For now, the only thing we can do is adding as much as possible tests to guarantee the commits on the master are more "stable". So since all commits on the master passed the test, they are equally "stable".

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