Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Decouple instruction decoding from emulation unit #79

Merged
merged 1 commit into from
Nov 7, 2022

Conversation

Risheng1128
Copy link
Collaborator

@Risheng1128 Risheng1128 commented Nov 3, 2022

To implement jit compiler in emulator, need to separate the part of decode instruction from execution because we have to fetch instructions continuously.

@lgtm-com
Copy link

lgtm-com bot commented Nov 3, 2022

This pull request introduces 1 alert when merging 244e3a3 into a550597 - view on LGTM.com

new alerts:

  • 1 for FIXME comment

@jserv jserv changed the title Separate the part of decode from execution Decouple instruction decoding from emulation unit Nov 3, 2022
src/decode.c Outdated Show resolved Hide resolved
src/decode.c Outdated Show resolved Hide resolved
src/decode.h Outdated Show resolved Hide resolved
src/emulate.c Outdated Show resolved Hide resolved
src/decode.h Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
src/decode.c Outdated Show resolved Hide resolved
src/decode.c Outdated Show resolved Hide resolved
src/decode.c Outdated Show resolved Hide resolved
src/decode.c Outdated

#include "decode.h"

static inline bool decode_load(struct rv_insn_t *rv_insn, uint32_t insn)
Copy link
Collaborator

@RinHizakura RinHizakura Nov 3, 2022

Choose a reason for hiding this comment

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

I am not sure if it makes sense to give each instruction a single decode function. Since the same type of instruction (e.g. I-type, S-type, ...) should share the same decoding logic. I think that dealing with each instruction with an independent function introduces a lot of mess.

What you should do is define every type of decoding function, for example:

static inline bool decode_i_type(struct rv_insn_t *rv_insn, uint32_t insn)
{
    /* I-type
     *  31       20 19   15 14    12 11   7 6      0
     * | imm[11:0] |  rs1  | funct3 |  rd  | opcode |
     */
    rv_insn->imm = dec_itype_imm(insn);
    rv_insn->rs1 = dec_rs1(insn);
    rv_insn->funct3 = dec_funct3(insn);
    rv_insn->rd = dec_rd(insn);
}

After that, you can fix the jump table or the label for compute-goto to jump to the correct types of decoding functions.

Copy link
Collaborator Author

@Risheng1128 Risheng1128 Nov 3, 2022

Choose a reason for hiding this comment

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

Thanks for your reminder! I think I will try to rewrite as following for every instruction type

static inline bool op_load(struct rv_insn_t *ir, uint32_t insn)
{
    /* decode I-type */
    decode_i_type(ir, insn);
    /* dispatch operation type */
    switch (ir->funct3) {
    ...
    }
    return true;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@Risheng1128, During code review, you should address the suggested improvements rather than simply responding "Thanks" as spam. That is, "show me the code" or response with something conceptual, which is ALWAYS better than nothing.

@Risheng1128
Copy link
Collaborator Author

Risheng1128 commented Nov 7, 2022

riscv-arch-test result: RV32IMC and privilege situations pass.

TODO:

  • Refine the commit message
  • Use "computed goto" in function "rv_emulate"

@lgtm-com
Copy link

lgtm-com bot commented Nov 7, 2022

This pull request introduces 1 alert and fixes 1 when merging 11f02d5 into 3305664 - view on LGTM.com

new alerts:

  • 1 for FIXME comment

fixed alerts:

  • 1 for FIXME comment

@jserv jserv changed the base branch from master to wip/instruction-decode November 7, 2022 17:16
@jserv jserv merged commit 549f069 into sysprog21:wip/instruction-decode Nov 7, 2022
@Risheng1128 Risheng1128 deleted the decode branch November 7, 2022 20:56
vestata pushed a commit to vestata/rv32emu that referenced this pull request Jan 24, 2025
This commit breaks RISC-V instruction decoding and emulation into separate
stage, meaning that it is feasible to incorporate further IR optimizations
and JIT code generation.
                                                                                                                                                              
RISC-V Architecture Test: RV32I/M/C/privilege pass.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants