RFR: 8340732: RISC-V: Refactor crc32 scalar version
Fei Yang
fyang at openjdk.org
Wed Sep 25 07:28:37 UTC 2024
On Tue, 24 Sep 2024 07:22:51 GMT, Hamlin Li <mli at openjdk.org> wrote:
> Hi,
> Can you help to review this patch?
> As discussed in https://github.com/openjdk/jdk/pull/20910#discussion_r1755150447, it's helpful to refactor the existing scalar version of crc32 intrinsic.
> Several refactoring are done in this pr,
> 1. Simplify the `len` usage, now it only decreases (i.e. change in one direction)
> 2. Simplify the code paths
> 3. Remove one instruction in `L_by4_loop`
> 4. Remove unnecessary code
> 5. Other misc
>
> Thanks!
Generally looks fine. I only have minor comments.
src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 1621:
> 1619:
> 1620: bind(L_by1_loop);
> 1621: blez(len, L_exit);
Since `len` will never be lower than zero, maybe `beqz` is more accurate here?
src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 1627:
> 1625: andi(tmp2, tmp1, right_8_bits);
> 1626: update_byte_crc32(crc, tmp2, table0);
> 1627: blez(len, L_exit);
Same question here.
src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 1633:
> 1631: andi(tmp2, tmp2, right_8_bits);
> 1632: update_byte_crc32(crc, tmp2, table0);
> 1633: blez(len, L_exit);
and here.
src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 6097:
> 6095: __ kernel_crc32(crc, buf, len,
> 6096: c_rarg3, c_rarg4, c_rarg5, c_rarg6, // tmp's for tables
> 6097: c_rarg7, t2, x28, x29, x30, x31); // misc tmps
Nit: Can we use alias t3-t6 for x28-x31 instead? The purpose is to be consistent with `t2` in naming.
-------------
PR Review: https://git.openjdk.org/jdk/pull/21150#pullrequestreview-2325373678
PR Review Comment: https://git.openjdk.org/jdk/pull/21150#discussion_r1774679248
PR Review Comment: https://git.openjdk.org/jdk/pull/21150#discussion_r1774679913
PR Review Comment: https://git.openjdk.org/jdk/pull/21150#discussion_r1774680149
PR Review Comment: https://git.openjdk.org/jdk/pull/21150#discussion_r1773414691
More information about the hotspot-dev
mailing list