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