Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings
This repository was archived by the owner on Jan 25, 2022. It is now read-only.

Implement the compressed instructions #5

Open
eriktai wants to merge 24 commits into sysprog21:master
base: master
Choose a base branch
Loading
from eriktai:master

Conversation

Copy link

@eriktai eriktai commented Jan 11, 2020

Some modification of rv32emu to support C ext.

  1. Add a new variable named insn_type to distinguish the 16-bit and 32-bit instruction.
  2. Modify the unsigned char get_insn32(uint32_t, uint32_t*) to return the type of instruction and then assign the pointer the machine code of the instruction.
  3. Modify the instruction offset, instead of always proceeding 4 bytes, it will depend on the type of instruction.
    • 16-bits : 2 bytes
    • 32-bits : 4 bytes
  4. Rearrange the offset of outputing the signature.
  5. Update the comparing function, which can compare the groundtruth and output of signature of this emulator for debugging.
  6. It is also the version of fixing start address of stack pointer.
  7. Imp and finish the test of all 32-bit version compressed instructions except those 64-bit version, 128-bit version and F ext.

Future work

  • I would like to imp the floating point extension.

Copy link
Contributor

jserv commented Jan 11, 2020

Remove the generated files and rework!

CSRs.h Outdated
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

The range from Line 1 to 68 is not necessary. Remove these lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Author

Choose a reason for hiding this comment

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

Sor, I may forget about it.

Copy link
Contributor

jserv commented Jan 11, 2020

Hint: use git rebase -i to rework these Git commits.

Copy link
Author

eriktai commented Jan 11, 2020 •
edited
Loading

Sorry, I have a question.
Do I need to rework all the commit messages?

Copy link
Contributor

jserv commented Jan 11, 2020

Sorry, I have a question.
Do I need to rework all the commit messages?

Yes, all commits should be reworked. You shall squash the similar changes and make the commits clear.

Copy link
Author

eriktai commented Jan 11, 2020

Hello, Jserv
I have reworked these commit messages.
Please check, thank you~

Copy link
Contributor

jserv commented Jan 11, 2020

After reworking Git commits with git rebase -i, you should perform git push --force for this branch.

@@ -1,19 +1,60 @@
BINS = emu-rv32i test1
TEST_TARGETS = \
Copy link
Contributor

Choose a reason for hiding this comment

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

Send another pull request for adding RISC-V test cases.

Copy link
Contributor

jserv commented Jan 11, 2020

Ensure no generated files such as ELF, log, dump files in Git repository.

Copy link
Contributor

jserv commented Jan 12, 2020

You have to rebase master branch of upstream. And then, force push.

Copy link
Contributor

jserv commented Jan 12, 2020

Don't use git merge. Use git rebase instead.

Copy link
Contributor

jserv commented Jan 12, 2020

Reference instructions:

  1. set upstream: git remote add upstream https://github.com/sysprog21/rv32emu && git fetch upstream
  2. Perform rebase: git rebase upstream/master
  3. Fix the conflicts.

CSRs.h Outdated

#include <stdint.h>


Copy link
Contributor

Choose a reason for hiding this comment

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

Can you introduce a dedicated structure for preserving the following fields?

emu-rv32i.c Outdated
After this you can run it with the emulator like this:
emu-rv32i zephyr/zephyr.elf
original copyright:
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the range from Line 1 to Line 35.

emu-rv32i.c Outdated
// #define DEBUG_OUTPUT
// #define DEBUG_EXTRA
#define DEBUG_OUTPUT
#define DEBUG_EXTRA
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't do that.

emu-rv32i.c Outdated
#define DEBUG_EXTRA

#if 1
#define RV32C
Copy link
Contributor

Choose a reason for hiding this comment

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

RV32C should be assigned via Makefile

emu-rv32i.c Outdated
""};
#define C_STATS_NUM 64

#ifndef RV32C
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to use the following:

char statnames[STATS_NUM + C_STATS_NUM][16] = {
 ... /* RV32I instructions */
#if defined(RV32C)
 ... /* RV32C specific instructions */
#endif
}

Copy link
Author

Choose a reason for hiding this comment

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

OK! It's more structural.

emu-rv32i.c Outdated
maxmemr = maxmemw = 0;
for(int i=0;i<STATS_NUM;i++) stats[i]=0;
#ifdef RV32C
for(int i=0;i<C_STATS_NUM;i++) stats[i+64] = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto. You can define macro for the range. Thus, we don't have to duplicate the loops.

Copy link
Contributor

jserv commented Jan 12, 2020

Before submitting the changes, ensure the completion of executing clang-format -i *.[ch]

@jserv jserv changed the title (削除) Finishing the compressed instruction of rv32emu (削除ここまで) (追記) Implement the compressed instructions (追記ここまで) Jan 16, 2020
Copy link
Contributor

jserv commented Jan 16, 2020

Reworked yet?

Copy link
Author

eriktai commented Jan 19, 2020 via email

Not yet, I will update it tomorrow. Jim Huang <notifications@github.com> 於 2020年1月17日 週五 上午1:10寫道:
...
Reworked yet? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <sysprog21/rv32emu#5?email_source=notifications&email_token=AI4KYTPOEE7CU32GFLJLIVDQ6CIGPA5CNFSM4KFQ6NU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJE2HLQ#issuecomment-575251374>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI4KYTOCJ6566MQXVGGPNSDQ6CIGPANCNFSM4KFQ6NUQ> .

eriktai and others added 17 commits January 20, 2020 22:09 
Add the method of how to test file
Delete the unnecessary file
Copy link
Author

eriktai commented Jan 20, 2020

Hello Jserv,
Sorry about the absence a few days ago.
I have rebased all commits.

sig.txt
rom.v
debug.txt
CSR.h
Copy link
Contributor

Choose a reason for hiding this comment

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

CSR.h was tracked in GIT repository.

@@ -23,6 +24,8 @@
// #define DEBUG_OUTPUT
// #define DEBUG_EXTRA


/* added by Shaos: */
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to mention the contributor name inside source file.

@@ -140,7 +163,7 @@ uint64_t mtimecmp;
/* virtual start address for index 0 in the ram array */
uint32_t ram_start;

/* program entry point */
/* Program entry point */
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't do that. Let's minimize the changes.

/* CPU state */
uint32_t pc;
uint32_t next_pc;
uint32_t insn;
uint32_t reg[32];


Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto. Minimize the changes.

Copy link
Contributor

jserv commented Jan 20, 2020

Thank @kksweet8845 again for contributing to this project. However, we shall always merge commits with better Git commit messages for further development. Please split this pull request into the following:

  1. New pull request for rv32i conformance tests;
  2. Once the above is merged, modify the pull request to concentrate on RV32C;

Changes regarding CSR instructions should be addressed in part 1.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@jserv jserv jserv left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants

AltStyle によって変換されたページ (->オリジナル) /