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