On Tue, 26 Nov 2024 18:11:59 GMT, Quan Anh Mai <qa...@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
