RFR: 8322770: Implement C2 VectorizedHashCode on AArch64 [v9]

Andrew Haley aph at openjdk.org
Wed Sep 18 13:21:19 UTC 2024


On Wed, 18 Sep 2024 12:48:37 GMT, Mikhail Ablakatov <duke at openjdk.org> wrote:

>> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 5411:
>> 
>>> 5409:                                                                                    : -1;
>>> 5410:     guarantee(multiply_by_halves != -1, "unknown multiplication algorithm");
>>> 5411: 
>> 
>> It's hard to follow this logic. Try something like this:
>> Suggestion:
>> 
>> 
>>     Assembler::SIMD_Arrangement load_arrangement;
>>     switch (eltype) {
>>       case T_BOOLEAN:
>>       case T_BYTE:
>>         load_arrangement = T8B;
>>         multiply_by_halves = true;
>>         break;
>>       case T_CHAR:
>>       case T_SHORT:
>>         load_arrangement = T8H;
>>         multiply_by_halves = true;
>>         break;
>>       case T_INT:
>>         load_arrangement = T4S;
>>         multiply_by_halves = false;
>>         break;
>>       default:
>>         ShouldNotReachHere();
>>     }
>
> The current implementation reflects that the decision to process a register by halves depends on the arrangement used. In the previous version of this PR, we tested for `load_arrangement` in places where `multiply_by_halves` is tested now. This way, for example, changing the arrangement for `T_CHAR`/`T_SHORT` from `T8H` to `T4H` requires only changing the arrangement itself. Using the logic you suggest would require one to be aware of the connection between `load_arrangement` and `multiply_by_halves` that must be maintained. Therefore, I recommend leaving the code as it is.

No, because the connection between load_arrangement and multiply_by_halves is inherent in the logic. Please keep things as simple as possible for this implementation. If a future engineer decides to extend this code they'll probably do something different. Speculating here about what might happen is over-engineering.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18487#discussion_r1765043415


More information about the hotspot-dev mailing list