RFR: 8310691: [REDO] [vectorapi] Refactor VectorShuffle implementation [v2]

[Paul Sandoz]

On Tue, 26 Nov 2024 18:11:59 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

>> src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Int256Vector.java line 870:
>> 
>>> 868:         @Override
>>> 869:         public final Int256Shuffle rearrange(VectorShuffle<Integer> shuffle) {
>>> 870:             return (Int256Shuffle) toBitsVector().rearrange(((Int256Shuffle) shuffle)
>> 
>> I think the cast is redundant for all vector kinds. Similarly the explicit cast is redundant for the integral vectors, perhaps in the template separate out the expressions to avoid it where not needed?
>> 
>> We could also refer to `VSPECIES` directly rather than calling `vspecies()`, same applies in other methods in the concrete vector classes.
>
> The cast is added so that we have the concrete type of the shuffle, the result of `toShuffle` is only `VectorShuffle<Integer>`

Ah i see now, you want to ensure an invocation to the final/concrete method. (The IDE's highlighting of the redundant cast is misleading)

>> src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Int256Vector.java line 908:
>> 
>>> 906:         }
>>> 907: 
>>> 908:         private static boolean indicesInRange(int[] indices) {
>> 
>> Since this method is only called from an assert statement in the constructor we could avoid the clever checking that assertions are enabled and the explicit throwing on an AssertionError by using a second expression that produces an error message when the assertion fails : e.g.,
>> 
>>             assert indicesInRange(indices) : outOfBoundsAssertMessage(indices);
>
> Yes you are right, since this method is only called in `assert` I think we can just remove the `assert` trick inside instead.

That's fine too.

>> src/jdk.incubator.vector/share/classes/jdk/incubator/vector/IntVector.java line 2473:
>> 
>>> 2471:     final <F>
>>> 2472:     VectorShuffle<F> toShuffle(AbstractSpecies<F> dsp, boolean wrap) {
>>> 2473:         assert(dsp.elementSize() == vspecies().elementSize());
>> 
>> Even though we force inline I cannot quite decide if it is better to forego the assert since it unduly increases method size. Regardless it may be useful to place the partial wrapping logic in a separate method, given it is less likely to be used.
>
> You don't have to worry too much about inlining of Vector API methods since it is done during late inlining and we have a pretty huge budget there.

Ok, my comment was motivated by some feedback on the FFM API where IIRC forced inline limits were reached.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21042#discussion_r1872036603
PR Review Comment: https://git.openjdk.org/jdk/pull/21042#discussion_r1872037914
PR Review Comment: https://git.openjdk.org/jdk/pull/21042#discussion_r1872037711