On Tue, 29 Oct 2024 20:27:20 GMT, Paul Sandoz <psan...@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
