On Tue, 20 Feb 2024 07:35:28 GMT, Emanuel Peter <epe...@openjdk.org> wrote:
>> Jatin Bhateja has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review comments resolutions.
>
> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1584:
>
>> 1582: Label *larr[] = {&case0, &case1, &case2, &case3};
>> 1583: for (int i = 0; i < 4; i++) {
>> 1584: // dst[i] = mask ? src[index[i]] : 0
>
> I like these comments a lot!
> They would be even better if they had a more clear relation to the register
> names.
>
> `dst[i] = mask[i+midx] ? src[idx_base[i]] : 0`
> It would then be helpful to know the types of these arrays.
> It seems that `idx_base` is an array with type int, right?
Ah, and can we use `base_index`? Otherwise we have an inconsistency with
`index` vs `idx`.
> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1624:
>
>> 1622: jccb(Assembler::carryClear, *larr[i]);
>> 1623: movl(rtmp, Address(idx_base, i * 4));
>> 1624: addl(rtmp, offset);
>
> This is really the only line that is additional to the code in
> `vgather8b_masked`. Why not just put it under a `bool has_offset` flag, and
> then you ran just reuse this code here from `vgather8b_masked`, and reduce
> code duplication?
Ok, you basically are implementing the cross-product of gather with `offset`
and `mask`.
Why not have 2 flags then, and a bit more comments to explain?
> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1716:
>
>> 1714: XMMRegister xtmp3, Register
>> rtmp,
>> 1715: Register midx, Register length,
>> 1716: int vector_len, int vlen_enc) {
>
> I would like to see more descriptive names, where I don't have to
> reverse-engineer their meaning.
> What are the pre/post-conditions on `midx`?
I'll rereview after
> src/hotspot/cpu/x86/x86.ad line 4143:
>
>> 4141: %}
>> 4142:
>> 4143: instruct vgather_subwordLE8B_off(vec dst, memory mem, rRegP idx, rRegI
>> offset, rRegP tmp, rRegI rtmp, rFlagsReg cr) %{
>
> Suggestion:
>
> instruct vgather_subwordLE8B_offset(vec dst, memory mem, rRegP idx, rRegI
> offset, rRegP tmp, rRegI rtmp, rFlagsReg cr) %{
There are more instances
> src/hotspot/cpu/x86/x86.ad line 4147:
>
>> 4145: match(Set dst (LoadVectorGather mem (Binary idx offset)));
>> 4146: effect(TEMP tmp, TEMP rtmp, KILL cr);
>> 4147: format %{ "vector_gatherLE8 $dst, $mem, $idx, $offset\t! using $tmp
>> and $rtmp as TEMP" %}
>
> Suggestion:
>
> format %{ "vector_gatherLE8_offset $dst, $mem, $idx, $offset\t! using $tmp
> and $rtmp as TEMP" %}
There may be more instances
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1495386503
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1495364719
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1495416109
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1495426766
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1495426957
