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

[Quan Anh Mai]

On Tue, 29 Oct 2024 20:27:20 GMT, Paul Sandoz <psandoz at openjdk.org> wrote:

>> Quan Anh Mai has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit:
>> 
>>   [vectorapi] Refactor VectorShuffle implementation
>
> src/jdk.incubator.vector/share/classes/jdk/incubator/vector/AbstractVector.java line 228:
> 
>> 226:         }
>> 227: 
>> 228:         AbstractVector<?> iota = vspecies().asIntegral().iota();
> 
> I suspect the non-power of two code is more efficient. (Even better if the MUL could be transformed to a shift for power of two values.)
> 
> Separately, it makes me wonder if we should revisit the shuffle factories if it is now much more efficient to construct a shuffle from a vector.

`shuffleFromOp` is a slow path op so I don't think it is. Additionally, our vector multiplication is against a scalar, too. So we can optimize it if `step` is a constant.

> 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>`

> 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.

> 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.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21042#discussion_r1859037153
PR Review Comment: https://git.openjdk.org/jdk/pull/21042#discussion_r1859033054
PR Review Comment: https://git.openjdk.org/jdk/pull/21042#discussion_r1859033749
PR Review Comment: https://git.openjdk.org/jdk/pull/21042#discussion_r1859032221