RFR: 8334554: RISC-V: verify & fix perf of string comparison [v3]

[Robbin Ehn]

On Tue, 25 Jun 2024 15:20:36 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Hi,
>> Can you help to review this patch?
>> Thanks!
>> 
>> As in compare-UL/LU, it already uses m4, so in this patch also use m4 for compare-UU/LL.
>> 
>> ## Test
>> tested on K230-CanMV, vlen = 128.
>> warmup: 10 times
>> iteration: 10 times
>> 
>> ### Before patch
>> <google-sheets-html-origin style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-style: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;">
>> Benchmark | (size) | Score+rvv | Score-rvv | -rvv/+rvv
>> -- | -- | -- | -- | --
>> com.arm.benchmarks.intrinsics.StringCompareToDifferentLength.compareToLL | 24 | 4242936.876 | 7227607.14 | 1.703444419
>> com.arm.benchmarks.intrinsics.StringCompareToDifferentLength.compareToLL | 36 | 5738695.363 | 8157070.353 | 1.421415468
>> com.arm.benchmarks.intrinsics.StringCompareToDifferentLength.compareToLL | 72 | 7163243.984 | 7209568.036 | 1.00646691
>> com.arm.benchmarks.intrinsics.StringCompareToDifferentLength.compareToLL | 128 | 8627566.301 | 12720927.51 | 1.474451435
>> com.arm.benchmarks.intrinsics.StringCompareToDifferentLength.compareToLL | 256 | 14632020.04 | 16291127.26 | 1.113388802
>> com.arm.benchmarks.intrinsics.StringCompareToDifferentLength.compareToLL | 512 | 26539410.59 | 23612505.95 | 0.8897147833
>> com.arm.benchmarks.intrinsics.StringCompareToDifferentLength.compareToLU | 24 | 4913490.894 | 10454585.94 | 2.127730807
>> com.arm.benchmarks.intrinsics.StringCompareToDifferentLength.compareToLU | 36 | 7230036.286 | 13561865.48 | 1.875767277
>> com.arm.benchmarks.intrinsics.StringCompareToDifferentLength.compareToLU | 72 | 9525418.104 | 21901656.51 | 2.299285582
>> com.arm.benchmarks.intrinsics.StringCompareToDifferentLength.compareToLU | 128 | 12645301.4 | 37351484.04 | 2.953783611
>> com.arm.benchmarks.intrinsics.StringCompareToDifferentLength.compareToLU | 256 | 21147886.68 | 64886475.43 | 3.068225039
>> com.arm.benchmarks.intrinsics.StringCompareToDifferentLength.compareToLU | 512 | 39738017.94 | 125169103.6 | 3.149857745
>> com.arm.benchmarks.intrinsics.StringCompareToDifferentLength.compareToUL | 24 | 5183884.427 | 11040441.7 | 2.129762314
>> com.arm.benchmarks.intrinsics.StringCompareToDifferentLength.compareToUL | 36 | 7421224.1 | 13879329.16 | 1.870221
>> com.arm.benchmarks.intrinsics.StringCompareToDifferentLength.compareToUL | 72 | 9739241.916 |...
>
> Hamlin Li has updated the pull request incrementally with one additional commit since the last revision:
> 
>   misc

Thanks, looks good!

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 2361:

> 2359:   mv(result, false);
> 2360: 
> 2361:   element_compare(a1, a2, result, cnt, tmp1, tmp2, v2, v4, v2, true, DONE, Assembler::m2);

It's not apparent to me why you use m2 here. Can you add a comment?

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 2414:

> 2412:   la(a2, Address(a2, base_offset));
> 2413: 
> 2414:   element_compare(a1, a2, result, cnt1, tmp1, tmp2, v2, v4, v2, elem_size == 1, DONE, Assembler::m2);

It's not apparent to me why you use m2 here. Can you add a comment?

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

Marked as reviewed by rehn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19825#pullrequestreview-2150520342
PR Review Comment: https://git.openjdk.org/jdk/pull/19825#discussion_r1660530930
PR Review Comment: https://git.openjdk.org/jdk/pull/19825#discussion_r1660530766