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