Skip to content

Support RISC-V Compressed Instructions #11

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

Merged
merged 2 commits into from
Jan 17, 2022
Merged

Conversation

xiaohan484
Copy link
Contributor

This work is based on sammer1107 and ccs100203. The RV32C is integrated with COMPUTE_GOTO and still don't support RV32C.F instruction. The C-extension compliance test was passed.

@jserv
Copy link
Contributor

jserv commented Jan 7, 2022

You should rebase the latest master branch. See https://git-scm.com/docs/git-rebase
Also, decouple RISC-V Architecture Test from this pull request. That is, this pull request should only implement the revised RV32C instruction support.

@jserv jserv requested review from eecheng87 and jserv January 7, 2022 03:29
Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Rework your git commits! See https://blog.yorkxin.org/posts/git-rebase.html

@jserv
Copy link
Contributor

jserv commented Jan 7, 2022

Files statistics.[ch] should appear in another pull request!

@eecheng87
Copy link
Collaborator

@jserv , should these commits be squashed into a single one?

@jserv
Copy link
Contributor

jserv commented Jan 7, 2022

@jserv , should these commits be squashed into a single one?

It depends on the scenario. In this case, at least two git commits should be landed:

  • C extension support
  • Tweaks for computed-goto

In addition, @xiaohan484 should list @ccs100203 as the co-author. See https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

@eecheng87
Copy link
Collaborator

@jserv, commits are squashed into 2 parts. @xiaohan484 will polish commit messages later.

@xiaohan484 xiaohan484 force-pushed the master branch 4 times, most recently from 9623615 to 706c3af Compare January 7, 2022 07:31
@eecheng87
Copy link
Collaborator

@xiaohan484 , I think "co-author" should be mentioned in commit body, not commit header.

@xiaohan484 xiaohan484 force-pushed the master branch 2 times, most recently from 8fc854b to 643cbc7 Compare January 7, 2022 08:35
@jserv
Copy link
Contributor

jserv commented Jan 7, 2022

In git commit messages, you should append the statement "No RV32C.F support."

@jserv
Copy link
Contributor

jserv commented Jan 14, 2022

I rename TABLE_TYPEC to COMPRESSED_TABLE_TYPE and add more comment for exception handler function.Is it suitable?

COMPRESSED_TABLE_TYPE is too long. How about TABLE_TYPE_RVC?

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Use "git rebase -i" to rework these commits.

@xiaohan484 xiaohan484 force-pushed the master branch 2 times, most recently from 9ed8ea4 to a7cee22 Compare January 15, 2022 06:41
@xiaohan484
Copy link
Contributor Author

I rebase the commit but remain the latest commit.The code "inst_len" parts are in "struct riscv_t", I move "enum" part to the above of "struct riscv_t" and indent the enum parts.

@jserv
Copy link
Contributor

jserv commented Jan 15, 2022

I rebase the commit but remain the latest commit.The code "inst_len" parts are in "struct riscv_t", I move "enum" part to the above of "struct riscv_t" and indent the enum parts.

No, you should use git rebase -i to split and rework into two git commits:

  1. Preliminary RV32C support
  2. Implement computed-goto path for RV32C

Of course, you have to ensure each git commit message meaningful and informative.

@xiaohan484 xiaohan484 force-pushed the master branch 2 times, most recently from bf73981 to 9a71fdd Compare January 16, 2022 06:14
Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Don't mention "RV32C.F excluded" in the subject of git commit messages.
You should write down the complete sentences rather than bullet points.

ccs100203 and others added 2 commits January 17, 2022 14:07
  There has a variable inst_len(2 or 4) to calculate the next PC, RV32C
  related exception handler and all RV32C implementation exclude RV32C.F.

  Note: c.ebreak does'nt pass compliance test.

Co-authored-by: ccs100203 <ccs100203@gmail.com>
Co-authored-by: Uduru0522 <kurasiki.homura@gmail.com>
  This work use TABLE_TYPE_RVC type for RV32C's opcode handler type
  to dispatch function.All RV32C related function rename to op_c* to
  cooperate with computed-goto for standard uncompressed function.
@jserv jserv merged commit bc8eb24 into sysprog21:master Jan 17, 2022
@jserv
Copy link
Contributor

jserv commented Jan 17, 2022

Thank @xiaohan484 for contributing! Let's move forward to riscv-arch-test compliance.

vestata pushed a commit to vestata/rv32emu that referenced this pull request Jan 24, 2025
Support RISC-V Compressed Instructions
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