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

rv32_i slli/srli/srai should not pseudo_op from rv64_i #111

Closed
ZenithalHourlyRate opened this issue May 3, 2022 · 4 comments
Closed

rv32_i slli/srli/srai should not pseudo_op from rv64_i #111

ZenithalHourlyRate opened this issue May 3, 2022 · 4 comments

Comments

@ZenithalHourlyRate
Copy link
Contributor

I'm trying to update rocket chip's Instructions.scala since the merge of #106. I found that SLLI_RV32, SRLI_RV32, SRAI_RV32 are not generated. It turns out that rv32_i uses the opcode from rv64_i directly

riscv-opcodes/rv32_i

Lines 1 to 3 in 833ba82

$pseudo_op rv64_i::slli slli rd rs1 shamtw 31..25=0 14..12=1 6..2=0x04 1..0=3
$pseudo_op rv64_i::srli srli rd rs1 shamtw 31..25=0 14..12=5 6..2=0x04 1..0=3
$pseudo_op rv64_i::srai srai rd rs1 shamtw 31..25=32 14..12=5 6..2=0x04 1..0=3

However, this would cause problem. In RV32 if the decoder is fed slli rd, rs, 32 as encoded below, the result would be slli rd, rs, 0 whereas originally this would cause an illegal instruction exception.

000000100000?????001?????0010011

Cc @neelgala

FYI

SLLI_RV32 = BitPat("b0000000??????????001?????0010011")
SLLI      = BitPat("b000000???????????001?????0010011")
@neelgala
Copy link
Collaborator

neelgala commented May 3, 2022

@ZenithalHourlyRate even prior to #106 the slli instruction was considered a pseudo op of the RV64 slli instruction (here). I have simply maintained that relationship here.

Right now the parse.py script is not going to dump any pseudo-ops into the code if the original instruction is already present. This is because there are instructions like slli in RV32 which have the same mnemonics as the original instruction in RV64 but vary in encodings. This makes it difficult to create unique entries in the instruction yaml (even some code for that matter) for each instruction.

If you want to build a decoder for rv32 you can use the following command : make EXTENSIONS="rv32* unratified/rv32*" which would generate the correct encoding for the use-case you have defined.

@ZenithalHourlyRate
Copy link
Contributor Author

If you want to build a decoder for rv32 you can use the following command : make EXTENSIONS="rv32* unratified/rv32*" which would generate the correct encoding for the use-case you have defined.

Thanks for this information! Unfortunately rocket chip uses the same inst.chisel for both RV32 and RV64, so if I want to make rocket chip work I need to generate both file and merge them, which is painful and not maintainable.

Right now the parse.py script is not going to dump any pseudo-ops into the code if the original instruction is already present.

Is it possible that the parse.py script could detect the difference in encoding and generate different entries when output encoding related codes. For mnemonics they could be the same.

@neelgala
Copy link
Collaborator

neelgala commented May 3, 2022

for both RV32 and RV64, so if I want to make rocket chip work I need to generate both file and merge them, which is painful and not maintainable.

If you are maintaining a single decoder for both RV32 and RV64 , then it would make sense to include the xlen checks in your decoder itself instead of overloading it in the inst.chisel. For example in rocket maybe moving this check inside this class would solve the problem ? This is similar to what spike does when encodings vary based on XLEN - it has further explicit encoding checks based on XLEN.

Is it possible that the parse.py script could detect the difference in encoding and generate different entries when output encoding related codes. For mnemonics they could be the same.

This is achievable but this would affect other artifacts as well - like the latex table generation. The instruction names in those tables will also change to SLLI_RV32_I which is not acceptable by the spec.

@ZenithalHourlyRate
Copy link
Contributor Author

For example in rocket maybe moving this check inside this class would solve the problem ?

Thanks! I will try to refactor that. Closing this issue now as it is deemed as an API change instead of a bug.

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

No branches or pull requests

2 participants