Wren6991 / Hazard3

3-stage RV32IMACZb* processor with debug
Apache License 2.0
696 stars 47 forks source link

BEXTM(I) Encoding Tables are Incorrect #17

Closed lenary closed 2 months ago

lenary commented 2 months ago

The following two custom instruction encoding tables are slightly wrong:

https://github.com/Wren6991/Hazard3/blob/a4412c0b00fd67788e42bc61599bf7a8c106d7b1/doc/sections/custom_extensions.adoc?plain=1#L102-L111

https://github.com/Wren6991/Hazard3/blob/a4412c0b00fd67788e42bc61599bf7a8c106d7b1/doc/sections/custom_extensions.adoc?plain=1#L154-L163

The issue is with bits 6:0 (the final two rows): these tables claim that:

When using the custom-0 opcode, according to Table 70 of the unpriviliged spec ("RISC-V base opcode map"), the following instruction encoding is described:

It looks like the problem here is that when you separated out the size field, you ended up repeating the bits from the opcode -- which implicitly encode the instruction size -- into your opc field.

The assembler directives using .insn I, 0x0b ... work correctly, because these directives put the 0xb value into bits 6:0, rather than knowing that an I-type instruction is always 32-bits and putting the 0xb value into bits 6:2 (and 0b11 into bits 1:0).

Wren6991 commented 2 months ago

Good spot, thank you. For avoidance of doubt the encoding used by hardware is this one:

https://github.com/Wren6991/Hazard3/blob/a4412c0b00fd67788e42bc61599bf7a8c106d7b1/hdl/rv_opcodes.vh#L151-L153

So as you say, the mistake in the table is it documents bits 1:0 twice. I'll fix the table.

Wren6991 commented 2 months ago

(also the comment in that header is outdated; the extension is Xh3bextm and it consists solely of these two instructions)

Wren6991 commented 2 months ago

I added a note on this to the v1.0 release notes. I'm not going to update the PDF as there are other docs changes ahead of this on the develop branch, and I don't want to add a dirty PDF build to the v1.0 release.

I will release a new PDF with the v1.1 release, which will contain this fix. It will document the same versions of all existing custom extensions as the v1.0 PDF (which for the record I consider to be extension versions 1.0).