riscv-opcodes
riscv-opcodes copied to clipboard
riscv
Can add checking for encoding conflict?
Hello,
For the standard/regular instructions, I think checking for encoding conflict is necessary. Note that only checking encoding conflict for instructions that belong to the same base extension. For example, c_ld and c_flw have the same encoding but no conflict because c_ld only exists on rv64 and c_flw only exists on rv32.
If that is ok, I can submit a patch for that checking.
And I also think it should be disabled that intrusions with the same name and same base extension to define multiple times.
Hello,
For the standard/regular instructions, I think checking for encoding conflict is necessary. Note that only checking encoding conflict for instructions that belong to the same base extension. For example,
c_ldandc_flwhave the same encoding but no conflict becausec_ldonly exists on rv64 andc_flwonly exists on rv32.If that is ok, I can submit a patch for that checking.
Yes.. I think if you can submit that patch it would be great. Basically iterating over the instr_dict after its creation here and checking for redundant encodings (should be a string-match maybe ?) should do the trick.
There is some amount of user-side caution that is expected while using opcodes in this directory. For, the exact case you mentioned the user shouldn't be including rv32_c_f and rv64_c extensions in the CLI arguments.
And I also think it should be disabled that intrusions with the same name and same base extension to define multiple times.
Ahh.. yes.. this is probably something I did not add explicitly - purely because once the first instance of the instruction is added.. the next instance is discarded (done here). So yes maybe an else clause to check if the base-extension/insrtuction match.. it throws out an error.
Appreciate you catching these and for volunteering - thanks
For, the exact case you mentioned the user shouldn't be including rv32_c_f and rv64_c extensions in the CLI arguments.
A note here is that the current make file includes both of them by default https://github.com/riscv/riscv-opcodes/blob/58a231eb2df5ac9a2e37b5e3548c564dc75b6e9b/Makefile#L1
yes.. that's because the encoding.out.h generated (and used by spike) needs to have all the instructions decoded (even if the encodings overlap) because spike has another layer of "decode"/"asserts" to choose which instructions to decode under certain conditions. So maybe the encoding checks should throw out a warning instead of an error maybe.. so that it goes through for those trying to do something similar to what spike wants to do.
For testing, we can add test/rv* files with artificial overlapping opcodes and check there are warning/error with GitHub action
Thank you for your comments. I add these checks on my fork repo and I didn't find any errors, so I think that checking should be an error for future typos. Here is my pull request.
I found this issue because I need to add some custom instructions and hope found some errors of duplicate encoding.