Conversation
TODO: need - release version of chisel 3.6.1, 5.1 - Zb/Zbk support - riscv-opcode path configurable
sequencer
left a comment
sequencer
left a comment
There was a problem hiding this comment.
some TODO need to be resolved, other LGTM(since I wrote it).
Need review by @jerryz123
| url = https://github.com/chipsalliance/chisel.git | ||
| [submodule "dependencies/rvdecoderdb"] | ||
| path = dependencies/rvdecoderdb | ||
| url = https://github.com/sequencer/rvdecoderdb.git |
There was a problem hiding this comment.
After @jerryz123 approves, I will transfer this repo to chipsalliance.
| // TODO: configurable | ||
| (org.chipsalliance.rvdecoderdb.fromFile.instructions(os.pwd / "dependencies" / "riscv-opcodes") ++ |
There was a problem hiding this comment.
can you fixup this TODO? with out configuring it, downstream users cannot use it.
|
|
||
| override def genTable(op: RocketDecodePattern): BitPat = op.instruction.name match { | ||
| case i if Seq("bne", "beq", "blt", "bltu", "bge", "bgeu", "sb", "sh", "sw", "add", "sub", "slt", "sltu", "and", "or", "xor", "sll", "srl", "sra", "czero.eqz", "czero.nez", "sfence.vma", "hfence.vvma", "hfence.gvma", "hsv.b", "hsv.h", "hsv.w", "sd", "addw", "subw", "sllw", "srlw", "sraw", "hsv.d", "mul", "mulh", "mulhu", "mulhsu", "div", "divu", "rem", "remu", "mulw", "divw", "divuw", "remw", "remuw", "amoadd.w", "amoxor.w", "amoswap.w", "amoand.w", "amoor.w", "amomin.w", "amominu.w", "amomax.w", "amomaxu.w", "lr.w", "sc.w", "amoadd.d", "amoswap.d", "amoxor.d", "amoand.d", "amoor.d", "amomin.d", "amominu.d", "amomax.d", "amomaxu.d", "lr.d", "sc.d", "custom0.rs1.rs2", "custom0.rd.rs1.rs2", "custom1.rs1.rs2", "custom1.rd.rs1.rs2", "custom2.rs1.rs2", "custom2.rd.rs1.rs2", "custom3.rs1.rs2", "custom3.rd.rs1.rs2").contains(i) => y | ||
| case i if Seq("amomaxu.w", "amoand.w", "amoor.w", "amoxor.w", "amoswap.w", "lr.w", "amomax.w", "amoadd.w", "amomin.w", "amominu.w", "sc.w", "lr.d", "amomax.d", "amoswap.d", "amoxor.d", "amoand.d", "amomin.d", "amoor.d", "amoadd.d", "amomaxu.d", "amominu.d", "sc.d", "hsv.w", "hsv.b", "hfence.vvma", "hsv.h", "hfence.gvma", "hsv.d", "or", "srl", "sltu", "sra", "sb", "add", "xor", "beq", "bge", "sw", "blt", "bgeu", "bltu", "bne", "sub", "and", "slt", "sh", "sll", "addw", "sd", "sllw", "sraw", "subw", "srlw", "mulhsu", "rem", "div", "mul", "mulhu", "mulh", "remu", "divu", "remuw", "divw", "divuw", "mulw", "remw", "sfence.vma", "czero.nez", "czero.eqz", "custom0.rs1.rs2", "custom0.rd.rs1.rs2", "custom1.rs1.rs2", "custom1.rd.rs1.rs2", "custom2.rs1.rs2", "custom2.rd.rs1.rs2", "custom3.rs1.rs2", "custom3.rd.rs1.rs2").contains(i) => y |
There was a problem hiding this comment.
How about split them with '\n'
jerryz123
left a comment
jerryz123
left a comment
There was a problem hiding this comment.
I do not feel confident in this change, as currently designed. The API change is very drastic.
We can keep the existing IntCtrlSigs bundle, and have a generator param control whether to use the new decoder (this can even be the default) or the old way.
I don’t think the reason keeping the original IntCtrl is a good design since for different instruction sets, some control signal will be optional. The current design doesn’t support it.
i don’t think the maintain burden is acceptable for me :( |
I don't like the premise that additional extensions require adding signals to IntCtrlSigs. IntCtrlSigs are the control signals for the subset of the control signals for the base integer ISA... the fields in this bundle should never need to change. We should develop interfaces and APIs that allow for extensions to define their own control signal bundles that propagate through the pipeline.
Aggressively deprecating APIs is a large maintenance burden. I don't see how retaining support for the original decoder is a large burden, if we do it with the correct interfaces. Once done, the original decoder code should never need to change again. I will look into integrating this without deprecating the existing decoder. |
3 participants
This PR takes over #3498 and removed Zb* and Zk* since #3532 has been merged.
Related issue: #3498
Type of change: other enhancement
Impact: API addition (no impact on existing code)
Development Phase: implementation
Release Notes
Switch to DecodeTable API.