RFR: 8320069: RISC-V: Add Zcb instructions [v3]

Fei Yang fyang at openjdk.org
Wed Jan 3 06:59:41 UTC 2024


On Wed, 20 Dec 2023 09:57:10 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> Hi, this is the instructions for zcb.
>> 
>> Due to over lack of infrastructure having multiple extension dependent instruction does not fit well.
>> Some of these compressed instructions are also missing 1 to 1 mapping, e.g. now we have a compressed not, but the corresponding instruction in uncompressed is still xor.
>> I think we need to do some rework here.
>> 
>> I also I don't like the macro expansion as it hopeless in debugger and 'IDE's (vim+rtags for me).
>> (macro stuff was originally done when templates where blacklisted in hotspot)
>> 
>> And I don't want an option for this, as zcb is coming in hwprobe, if you have compressed on you get them if they are supported (may depend on e.g. zbb).
>> 
>> I have done some modification since it passed tier1, so I'm running stuff over the weekend.
>
> Robbin Ehn has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Merge branch 'master' into zcb
>  - Merge branch 'master' into zcb
>  - zcb instruction set

Seems fine. I only have some minor comments.

src/hotspot/cpu/riscv/assembler_riscv.hpp line 542:

> 540:   INSN(_lbu, 0b0000011, 0b100); // Zcb
> 541:   INSN(_lh,  0b0000011, 0b001); // Zcb
> 542:   INSN(_lhu, 0b0000011, 0b101); // Zcb

The code comment for these three lines seems a bit misleading. These are normal 4-bytes encoding load/store instructions, not `Zcb` compressed instructions.

src/hotspot/cpu/riscv/assembler_riscv.hpp line 2962:

> 2960:   }
> 2961: 
> 2962:   // Format CU, c.[sz]ext.*, c.no

Nit: s/c.no/c.not/

src/hotspot/cpu/riscv/macroAssembler_riscv.hpp line 497:

> 495:   inline void zext_b(Register Rd, Register Rs) {
> 496:     if (do_compress_zcb(Rd, Rs) &&
> 497:         (Rd == Rs)) {

Nit: Maybe put the two conditions on the same line to be consistent in style with the other two `notr` & `zext_w` in the same file.
`if (do_compress_zcb(Rd, Rs) && (Rd == Rs)) {`

-------------

PR Review: https://git.openjdk.org/jdk/pull/17122#pullrequestreview-1801313328
PR Review Comment: https://git.openjdk.org/jdk/pull/17122#discussion_r1440102490
PR Review Comment: https://git.openjdk.org/jdk/pull/17122#discussion_r1440113088
PR Review Comment: https://git.openjdk.org/jdk/pull/17122#discussion_r1440026254


More information about the hotspot-dev mailing list