sail-riscv
sail-riscv copied to clipboard
riscv
C.ADDI disassembly inaccurate when immediate value is negative
The opcode 0x1541 should decode into c.addi x10, -16 (according to Spike). Sail is reporting it as c.addi x10, 48.
It seems to execute correctly; if x10 is 0, it gets written with 0xffffffff_fffffff0. It's only the immediate value shown in the disassembly that looks incorrect.
This is not a problem with the disassembly of C.ADDI; It appears to be a problem for every instruction with an immediate value.. @jrtc27 We can fix this particular mnemonic pretty easily by defining an signed variant - but it is a fair number of uses in some dozen or so files. Should we do this, or suffer along with a different disassembly? @ben-marshall: the crypto scalar has a bunch of immediate fields. Are the signed or unsigned or neither (in which case they're effectively unsigned)
Looking a bit deeper: the following instructions improperly decode signed constants as unsigned: [F]Loads/Stores, (LUI,AUIPC), JAL[R], btype ops (B**), Addi[w]/xori/ori/andi/slti[u], c.addi[w/16sp], c.[l[u]/and]i, c.j[al], c.bneq, c.beqz
I am assuming that crypto constants, hints, CSR#s, fences and shift amounts are not sign extended. There a bunch of compressed immediates that are specifically zero extended
That's 19 lines in 3 files: base, cext, fext (plus defining signed versions of the hex_bits_n functions, or explicitly sign extending in those 19 lines
That's 19 lines in 3 files: base, cext, fext
@allenjbaum It's remarkable that this remained undetected for several years.
We've known about it for years, just not a priority as it only affects the human-readable disassembly. If someone wants to fix it then great, but the string<->int handling is a bit nasty, so be warned...
You're right; it is rather...opaque (not good for something that's supposed to be readable and nasty code should be at least comments) and the hex_bits_n function don't seem to have anything to do with hex; it looks strictly binary to decimal string. I'd really prefer if it was hex (I'm not adept at reading Sail, so I could be misreading that... and no comments ....).
I tried using the dec_str function in string.sail, and it seems to work well. I'm wondering if it would be possible to solve this issue by making the disassembly display an optional feature or directly changing it to decimal representation. Admittedly, there would be quite a bit of work involved in modifying the corresponding references, but I would like to participate in completing this task. I'm unsure if there is a maintainer willing to accept my pull request (PR), but if it's possible, I will provide detailed explanations of the modifications in my PR. Or is there other thoughts on my proposal?
