RFR JDK-8223347 Integration of Vector API (Incubator): Java API, implementation, and tests

[Remi Forax]

Hi Paul,
about the API part (not the implementation),
there are location where the same concept is described with a different names which doesn't help to understand how thing work
- vector length <=> vector lane count
- vector shape <=> vector bits size
- element size <=> lane size

"size" should never be used (VectorSpecies.elementSize() by example ) because it's never clear if it's the byte size or the bits size, using byteSize and bitsSize as used several times in the API should be used everywhere.

Initially, having VectorSpecies and VectorShape confuse me, it's not the shape of the vector but only it's bits size,
renaming VectorShape to VectorBitsSize will help, i think. 

You have the same values defined at several places in the API (sometimes with different names), usually it's aliases but it makes the API bigger that it should.
I think all the reference to the element type, vector byte size, vector bit size, element byte size, element byte size, vector lane count should not appear on Vector because they are accessible from Species,
and to go from a vector to the corresponding species, species() is enough.

You have also the same methods defined at several places, on VectorSpecies and as a static method taking a species as parameter, 
all the methods of VectorSpecies that takes or return a vector/mask/shuffle should be static in vector/mask/shuffle.

I think Binary and Associative should be merged to one VectorOperation, given that the difference is subtle and the whole point of this API is that if the hardware do not provide a way to reduce a binary op, it can be done in Java. 

I have no idea what a Conversion.rangeType() is ?

More specially:
- VectorSpecies.loopbound() is not used in the example ?
- VectorSpecies.arrayType()/genericElementType() are more for implementers than users , no ? at least arrayType should be arrayElementType.
- VectorSpecies.withLanes() => withElementType()

- VectorMask.check* are implementer methods ?
- VectorMask.equal(VectorMask) => equiv

- VectorShuffle.check* are implementer methods ?
- VectorShuffle.vectorSpecies() => VectorShuffle.species()  (as in Vector and VectorMask)
- VectorShuffle.toVector() should return a IntVector
- VectorShuffle.laneIsValid(), it seems to be an operation on Vector, not VectorShuffle.

- IntVector.max, why there is no binary version that takes a mask ? It's documented but not why it's not available.


and i'm also not a big fans of the method that returns a long instead of a vector and only works on 64 bits values.

regards,
Rémi

----- Mail original -----
> De: "Paul Sandoz" <paul.sandoz at oracle.com>
> À: "core-libs-dev" <core-libs-dev at openjdk.java.net>, "hotspot-dev" <hotspot-dev at openjdk.java.net>
> Envoyé: Jeudi 2 Avril 2020 00:46:55
> Objet: RFR JDK-8223347 Integration of Vector API (Incubator): Java API, implementation, and tests

> Hi,
> 
> A prior email sent out a request for review of the Vector API in preparation for
> JEP 338: Vector API (Incubator) [1] to be proposed for target:
> 
>  https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-March/065345.html
>  <https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-March/065345.html>
> 
> This email expands the review of the API to the Java implementation and Java
> tests:
> 
>  http://cr.openjdk.java.net/~psandoz/panama/JDK-8223347-vector-api-integration-java/jdk_src_webrev/webrev/
>  <http://cr.openjdk.java.net/~psandoz/panama/JDK-8223347-vector-api-integration-java/jdk_src_webrev/webrev/>
> 
>  http://cr.openjdk.java.net/~psandoz/panama/JDK-8223347-vector-api-integration-java/jdk_test_webrev/webrev/
>  <http://cr.openjdk.java.net/~psandoz/panama/JDK-8223347-vector-api-integration-java/jdk_test_webrev/webrev/>
> 
> (Further emails will sent for review of general HotSpot changes CPU architecture
> specific HotSpot changes, x64 and aarch64, and performance tests.  For an early
> peek see the links in the description of JDK-8223347.)
> 
> —
> 
> The Vector API provides
> 
> - the public Vector class with vector operations common to all supported
> primitive types
> 
> - public primitive specializations, such as IntVector, with common operations
> associated with the primitive type
> 
> - internal concrete specializations for the vector size, such as Int256Vector,
> for a vector holding 8 ints.
> 
> Operations generally defer to one of approximately 20 vector intrinsics.  Some
> operations may be composed of other operations.
> 
> Explicit casts are performed by vector operations to ensure vectors arguments
> are of the required shape (bit size) and element type.
> 
> A vector intrinsic is an internal low-level vector operation. The last argument
> to the intrinsic is fall back behavior in Java, implementing the scalar
> operation over the number of elements held by the vector.  Thus, If the
> intrinsic is not supported in C2 for the other arguments then the Java
> implementation is executed (the Java implementation is always executed when
> running in the interpreter or for C1).
> 
> The public primitive specializations and the internal concrete implementations
> are generated from SSP template files, X-Vector.java.template and
> X-VectorBits.java.template respectively.
> 
> Overall the implementation approach is quite formulaic and rather repetitive.
> Once you grok the pattern It should be easier to review if a little laborious.
> 
> Unit tests are auto-generated by composing templates for each operation into an
> SSP template file which is then used to generate unit test files for each
> concrete vector class.  The tests are quite extensive and have found many a
> HotSpot issue in development across a wide range of platforms.
> 
> Paul.
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8201271
> <https://bugs.openjdk.java.net/browse/JDK-8201271>