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

[Fei Yang]

On Tue, 25 Jun 2024 12:10:22 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:
> 
>   refine lmul value

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

> 2453:     // Below construction of v regs and lmul is based on test on 2 different boards,
> 2454:     // vlen == 128 and vlen == 256 respectively.
> 2455:     if (!str1_isL && MaxVectorSize == 16) { // UU

Seems more readable to check `!encLL` instead of `!str1_isL`. As you see, we also pass this `encLL` to `element_compare` to indicate whether they are both Latin strings.

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.hpp line 40:

> 38:                        VectorRegister vr1, VectorRegister vr2,
> 39:                        VectorRegister vrs,
> 40:                        bool is_latin, Label& DONE, Assembler::LMUL lmul = Assembler::m2);

I think it's safer to pass this `lmul` param explictly at all call sites. So no need for a default param.

src/hotspot/cpu/riscv/riscv_v.ad line 3566:

> 3564: %}
> 3565: 
> 3566: instruct vstring_compareUVlen16(iRegP_R11 str1, iRegI_R12 cnt1, iRegP_R13 str2, iRegI_R14 cnt2,

Maybe instruct names like `vstring_compareU_128b` which I think is more general.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19825#discussion_r1652994128
PR Review Comment: https://git.openjdk.org/jdk/pull/19825#discussion_r1652970405
PR Review Comment: https://git.openjdk.org/jdk/pull/19825#discussion_r1652962903