search

riscv / riscv-bitmanip

Working draft of the proposed RISC-V Bitmanipulation extension
https://jira.riscv.org/browse/RVG-122
Creative Commons Attribution 4.0 International
204 stars 65 forks source link

Should slli.uw have opcode OP-IMM instead of OP-IMM-32? #147

Closed JamesKenneyImperas closed 3 years ago

JamesKenneyImperas commented 3 years ago

The current encoding for slli.uw with opcode OP-IMM-32 seems inconsistent to me. In the base architecture, this normally means that the encoded immediate (if a shift) is constrained to the range 0...31 (see section 5.2 in the Unprivileged ISA Specification). I think that here values 0...63 are allowed, in which case I would expect it to have opcode OP-IMM.

JamesKenneyImperas commented 3 years ago

Thinking about this more, I wonder whether the encoding should stay where it is but the shift amount should be constrained to 5 bits (0...31). Shifts in the range 32...63 are equivalent to slli.

ptomsich commented 3 years ago

While this doesn't contribute directly to resolving this, it is worth noting that the other .uw-instructions are OP-32.

aswaterman commented 3 years ago

It's six of one, half a dozen of the other. We need the OP-32 control signal to determine that we need to zap the MSBs of the input. OTOH, the illegal-instruction detection logic fits better in OP-XLEN, and we want the OP-XLEN control signal to write the full 64b result. Given the instruction doesn't fit neatly into either category, it doesn't really matter... and, either way, we're talking about ~1 gate on one path or the other.

Although none of this is likely to be on the critical path, the input conditioning needs to happen earlier than the output conditioning, and so given the choice, it makes sense to optimize for that (i.e. using OP-IMM-32). This is basically just a tiebreaker in my mind; it's not a major issue.

(I'll record this in the public review feedback, and hence will close this issue.)

JamesKenneyImperas commented 3 years ago

Hello Andrew,

Just to clarify what you are thinking: would you expect a shift in the range 32...63 to be an Illegal Instruction, or not? To me, it would seem more consistent with the base architecture if it was illegal, and there seems little point allowing these shifts since they are equivalent to slli anyway and hence redundant.

Earlier versions of the B extension specified shifts for GORCI/GREVIin an inconsistent way compared to the base, so I'm glad to see that this seems to have been fixed in this 1.0.0 version. slli.uw is the one remaining potential outlier in the up-for-ratification subsets I think.

James.

aswaterman commented 3 years ago

It's true that, for shift amounts >= 32, slli.uw is redundant with slli. It's basically harmless, though (no additional datapath cost). And the RV128 version of slli.uw will need all 7 bits, because only immediates 96-127 will be redundant with slli. So we can't reclaim that opcode bit.

I'll note this observation in the public review feedback.

JamesKenneyImperas commented 3 years ago

Ok, thanks. It seems a little odd to me that slli.uw allows shifts 0...63 when slliw is limited to 0...31, but as long as it is clear in the specification how the shamt is treated then this is fine.