RFR: 8322770: Implement C2 VectorizedHashCode on AArch64

Tue Aug 20 12:56:52 UTC 2024

On Sat, 3 Aug 2024 12:36:31 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> Hello,
>> 
>> Please review the following PR for [JDK-8322770 Implement C2 VectorizedHashCode on AArch64](https://bugs.openjdk.org/browse/JDK-8322770). It follows previous work done in https://github.com/openjdk/jdk/pull/16629 and https://github.com/openjdk/jdk/pull/10847 for RISC-V and x86 respectively. 
>> 
>> The code to calculate a hash code consists of two parts: a vectorized loop of Neon instruction that process 4 or 8 elements per iteration depending on the data type and a fully unrolled scalar "loop" that processes up to 7 tail elements.
>> 
>> At the time of writing this I don't see potential benefits from providing SVE/SVE2 implementation, but it could be added as a follow-up or independently later if required.
>> 
>> # Performance
>> 
>> ## Neoverse N1
>> 
>> 
>>   --------------------------------------------------------------------------------------------
>>   Version                                            Baseline           This patch
>>   --------------------------------------------------------------------------------------------
>>   Benchmark                   (size)  Mode  Cnt      Score    Error     Score     Error  Units
>>   --------------------------------------------------------------------------------------------
>>   ArraysHashCode.bytes             1  avgt   15      1.249 ?  0.060     1.247 ?   0.062  ns/op
>>   ArraysHashCode.bytes            10  avgt   15      8.754 ?  0.028     4.387 ?   0.015  ns/op
>>   ArraysHashCode.bytes           100  avgt   15     98.596 ?  0.051    26.655 ?   0.097  ns/op
>>   ArraysHashCode.bytes         10000  avgt   15  10150.578 ?  1.352  2649.962 ? 216.744  ns/op
>>   ArraysHashCode.chars             1  avgt   15      1.286 ?  0.062     1.246 ?   0.054  ns/op
>>   ArraysHashCode.chars            10  avgt   15      8.731 ?  0.002     5.344 ?   0.003  ns/op
>>   ArraysHashCode.chars           100  avgt   15     98.632 ?  0.048    23.023 ?   0.142  ns/op
>>   ArraysHashCode.chars         10000  avgt   15  10150.658 ?  3.374  2410.504 ?   8.872  ns/op
>>   ArraysHashCode.ints              1  avgt   15      1.189 ?  0.005     1.187 ?   0.001  ns/op
>>   ArraysHashCode.ints             10  avgt   15      8.730 ?  0.002     5.676 ?   0.001  ns/op
>>   ArraysHashCode.ints            100  avgt   15     98.559 ?  0.016    24.378 ?   0.006  ns/op
>>   ArraysHashCode.ints          10000  avgt   15  10148.752 ?  1.336  2419.015 ?   0.492  ns/op
>>   ArraysHashCode.multibytes        1  avgt   15      1.037 ?  0.001     1.037 ?   0.001  ...
>
>> > What is going on with C2_MacroAssembler::arrays_hashcode_elsize? It's a change that should not be in this patch.
>> 
>> It was a part of a cleanup. `C2_MacroAssembler::arrays_hashcode_elsize` was added by this PR, it's not present in [openjdk:master](https://github.com/openjdk/jdk/tree/master). JIC, please note that [mikabl-arm:285826-vmul-squashed](https://github.com/mikabl-arm/jdk/tree/285826-vmul-squashed) includes two commits: [mikabl-arm at f192030](https://github.com/mikabl-arm/jdk/commit/f19203015fb69e50636bdfa597c7aa48176a56cc) presented in the PR and [mikabl-arm at 3a52c7f](https://github.com/mikabl-arm/jdk/commit/3a52c7f89c293b79559201149f3159d5a8c831b6) / `HEAD` that develops further on the current state of the PR.
>> 
>> > Please take out every change that either does nothing or duplicates functionality that already exists. See type2aelembytes.
>> 
>> May I suggest to remove all _clenaup_ changes from [mikabl-arm:285826-vmul-squashed](https://github.com/mikabl-arm/jdk/tree/285826-vmul-squashed) and merge it to [mikabl-arm:8322770](https://github.com/mikabl-arm/jdk/tree/8322770) (this PR's branch) first? I can address any comments including `type2aelembytes` afterwards. This should make it easier to keep track of changes as now there are two different branches and I feel that this might get confusing.
> 
> I'm thinking that "clean
> 
>> > What is going on with C2_MacroAssembler::arrays_hashcode_elsize? It's a change that should not be in this patch.
>> 
>> It was a part of a cleanup. `C2_MacroAssembler::arrays_hashcode_elsize` was added by this PR, it's not present in [openjdk:master](https://github.com/openjdk/jdk/tree/master).
> 
> Oh, I see. I was assuming that this was a diff from master. I was in a hurry at the time...
> 
>> > Please take out every change that either does nothing or duplicates functionality that already exists. See type2aelembytes.
>> 
>> May I suggest to remove all _clenaup_ changes from [mikabl-arm:285826-vmul-squashed](https://github.com/mikabl-arm/jdk/tree/285826-vmul-squashed) and merge it to [mikabl-arm:8322770](https://github.com/mikabl-arm/jdk/tree/8322770) (this PR's branch) first? I can address any comments including `type2aelembytes` afterwards. This should make it easier to keep track of changes as now there are two different branches and I feel that this might get confusing.
> 
> It certainly is. If you can tell me the hashes of the code you want me to look at and the master commit it's based on it'll be much easier to see.
> 
> Looking past th...

Hello @theRealAph , this revision addresses most of the comments from https://github.com/mikabl-arm/jdk/commit/3a52c7f89c293b79559201149f3159d5a8c831b6 and merges changes from that branch to the PR branch. Thank you for taking a look!

Sorry for the force-push. Luckily, there were not so many review comments referencing the source code, so I hope this won't make the review process more difficult.

You may find current performance data in the commit message.

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

PR Comment: https://git.openjdk.org/jdk/pull/18487#issuecomment-2298790678