Review of v0.12

[christian-herber-nxp]

with the sole purpose of enabling 64-bit load/store instructions from the RV64 base ISA into RV32.

with the purpose of enabling load/store operations with even-odd register pairs.

This proposed extension doesn’t specify any new instructions per se, it only adds support for instructions which already exist in the 64-bit base ISA to RV32.

I would say it does. It defines new instructions in currently reserved encodings

Currently proposed instructions

You should be explicit about the encoding that these instructions use.

I think we need to give the instructions a real name, I am not sure if it would be ok to have two instructions with the same name which do somethign different wether its RV32/64.
Some brainstorming:
LW.D (load word double) - sort of inline with the proposed P specification, there the .D suffix is used for pair operations (RV32)
then you would define LD.Q for RV64.

Future RV64 instructions:

I wouldnt say this is future, I would just state in a footnote that the encoding these instructions reuse is not yet defined.

(Optional) compressed encodings

You might have to define Zcild extension for this.

This is of course incompatible with the (RV64) D extension, as those opcodes are then occupied by the double-precision load/store ops (C.FLD, C.FSD, C.FLDSP and C.FSDSP respectively). In that case, I think we should not try to have those compressed encodings, but just have them for RV32

Hm, gets more and more interesting. I think the chance of having an RV64 system with pair support and without floating point unit is relatively low. Almost

[tovine]

Good points, thank you very much for the review!

Regarding this one:

with the sole purpose of enabling 64-bit load/store instructions from the RV64 base ISA into RV32.

with the purpose of enabling load/store operations with even-odd register pairs.

I somehow want to emphasize that it's "just" adding the same operations as are already defined for RV64, like the next line says that a future RV64 extension could enable the 128-bit load/store operations form RV128.
Is there a middle ground where both these aspects could be balanced?

Regarding your second point: would this look better?

This proposed extension doesn't specify any new operations, but it takes currently reserved encodings in RV32 and adds support for instructions that already exist in the 64-bit base ISA - reusing the same encodings from RV64.

Currently proposed instructions

I think we need to give the instructions a real name, I am not sure if it would be ok to have two instructions with the same name which do somethign different wether its RV32/64.
Some brainstorming:
LW.D (load word double) - sort of inline with the proposed P specification, there the .D suffix is used for pair operations (RV32)
then you would define LD.Q for RV64.

Interesting points, I think we should hear from @aswaterman or someone else if this is a problem or not.
If possible I think keeping the same name would be easier from a "friction perspective" (how easy it would be to get acceptance). since it emphasizes that there's not really much new and controversial here, and maybe even less work for compilers and toolchains to get it supported (as they could just "enable" generation of these instructions instead of adding new support from scratch).
However I also see your point, and if we need to bring in new names then I think your suggestions are good.

Regarding Future RV64 instructions this kind of footnote could work, but after the points raised in #6 and riscv/riscv-isa-manual#1048 (comment) I'm a bit scared that pushing for these RV128 LQ/SQ operations to be defined could make it harder to get ratified (or at least fast-tracked).
If you (and Andrew et al) think that it should be relatively painless and uncontroversial I can give it a shot anyway - in the best case we could end up defining more of the RV128 base ISA in the process, and worst case we can pull that part of the proposal and save it for later if it gets a lot of pushback.

Regarding Zclsp/Zcild this sounds like the way to go, based on the work and discussions we had in https://github.com/riscv/riscv-code-size-reduction.
I have also added clarifications on the incompatibility with the Zcf and Zcd (C sub-)extensions (in the next draft, to be pushed once these questions have been resolved)

[christian-herber-nxp]

Good points, thank you very much for the review!

Regarding this one:

with the sole purpose of enabling 64-bit load/store instructions from the RV64 base ISA into RV32.

with the purpose of enabling load/store operations with even-odd register pairs.

I somehow want to emphasize that it's "just" adding the same operations as are already defined for RV64, like the next line says that a future RV64 extension could enable the 128-bit load/store operations form RV128. Is there a middle ground where both these aspects could be balanced?

My point was to start with the general. The goal is load store pair instructions. the fact that we are reusing currently reserved encoding space, is already the how, so i would pack it in a second sentence.

Regarding your second point: would this look better?

This proposed extension doesn't specify any new operations, but it takes currently reserved encodings in RV32 and adds support for instructions that already exist in the 64-bit base ISA - reusing the same encodings from RV64.

I would say that load store pair is a new operation. It is definitely a new instruction.

Currently proposed instructions

I think we need to give the instructions a real name, I am not sure if it would be ok to have two instructions with the same name which do somethign different wether its RV32/64.
Some brainstorming:
LW.D (load word double) - sort of inline with the proposed P specification, there the .D suffix is used for pair operations (RV32)
then you would define LD.Q for RV64.

Interesting points, I think we should hear from @aswaterman or someone else if this is a problem or not. If possible I think keeping the same name would be easier from a "friction perspective" (how easy it would be to get acceptance). since it emphasizes that there's not really much new and controversial here, and maybe even less work for compilers and toolchains to get it supported (as they could just "enable" generation of these instructions instead of adding new support from scratch). However I also see your point, and if we need to bring in new names then I think your suggestions are good.

Different ISAs handle such things differently. Arm will define a load store instruction, and define different operations for that. In RISC-V the compressed encodings have different instruction names as their uncompressed encoding.

On the other hand, looking at Z{f,d}inx, no new instruction names were given, the operation is different.

I think Z*inx is the better precedent, so this would actually suggest to stick with reusing the instruction names.

Regarding Future RV64 instructions this kind of footnote could work, but after the points raised in #6 and riscv/riscv-isa-manual#1048 (comment) I'm a bit scared that pushing for these RV128 LQ/SQ operations to be defined could make it harder to get ratified (or at least fast-tracked). If you (and Andrew et al) think that it should be relatively painless and uncontroversial I can give it a shot anyway - in the best case we could end up defining more of the RV128 base ISA in the process, and worst case we can pull that part of the proposal and save it for later if it gets a lot of pushback.

I would keep it in. If somebody has a strong opinion it shouldn't be there, you drop it. 32-bit pairs are what matters, the other thing is rather for saving work in the future in case it would be needed, or somebody finds it useful.

Regarding Zclsp/Zcild this sounds like the way to go, based on the work and discussions we had in https://github.com/riscv/riscv-code-size-reduction. I have also added clarifications on the incompatibility with the Zcf and Zcd (C sub-)extensions (in the next draft, to be pushed once these questions have been resolved)

I am of the opinion that compressed encoding for 64-bit pairs are not as important, and shouldn't overlap with F/D encodings.