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

[Paul Sandoz]

Hi Remi,

Thanks for the feedback. I am gonna need to go over this with John. Some comments inline.


> On Apr 20, 2020, at 6:31 AM, Remi Forax <forax at univ-mlv.fr> wrote:
> 
> 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.

I agree with being explicit here e.g. s/Vector.elementSize/Vector.elementBitSize and other usages.

I would prefer to consistently use length in methods to refer to the lane count, it matches more closely to an array and for thinking about bounds checking.

How about on Vector:

/**
 * Returns number of vector lanes ({@code VLENGTH}), commonly referred to as the lane count.
 *
 * @return the number of vector lanes
 */
public abstract int length();

And then change VectorShape.laneCount to VectorShape.laneLength.

?


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

Shape is quite a fundamental concept specified and used in the API, representing the information capacity of a vector.

I think it would be misleading to refer to it only as VectorBitSize and unwieldy to change the specification accordingly. In this sense it serves as a useful abstraction to talk about vectors of the same bit size, and operations that change that size or not (such as shape-changing or shape-invariant).



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

But it also makes it less usable, esp. for length().


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

It’s a little more nuanced, but I agree there is some duplication e.g. VectorSpecies.fromByteArray compared to the richer set of methods defined on [E]Vector. I think we can remove that method from VectorSpecies. Then I think we are dealing with the more “erased/reflective” and long carrier accepting methods for general operation without appealing to E.


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

Not quite true, ARM/x86 implementations do provide intrinsic implementations (leveraging a sequence of instructions) for some reduction operations e.g. min/max. I cannot recall there current state of the ARM/x86 implementations for other reductive operations but the plan is to optimize all of them at some point. Further, hardware may evolve in the future.


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

It’s the element type of the resulting output vector,

Doc on VectorOperators.Operator:

/**
 * Reports the special return type of this operator.
 * If this operator is a boolean, returns {@code boolean.class}.
 * If this operator is a {@code Conversion},
 * returns its {@linkplain Conversion#rangeType range type}.
 *
 * Otherwise, the operator's return value always has
 * whatever type was given as an input, and this method
 * returns {@code Object.class} to denote that fact.
 * @return the special return type, or {@code Object.class} if none
 */
Class<?> rangeType();


> More specially:
> - VectorSpecies.loopbound() is not used in the example ?

In the package-info?  Yes, we should update that.


> - VectorSpecies.arrayType()/genericElementType() are more for implementers than users , no ? at least arrayType should be arrayElementType.

I don’t think we need to expose genericElementType in the API.

I suspect arrayType may have been added before it was added to Class in 12 for the constable support.  I think we can remove it.


> - VectorSpecies.withLanes() => withElementType()
> 
> - VectorMask.check* are implementer methods ?

The intent is to provide abstractions so the developer can perform “reflective" checks on vector, mask, or shuffle.


> - VectorMask.equal(VectorMask) => equiv
> 

Because it’s too close to the name “equals”?


> - VectorShuffle.check* are implementer methods ?
> - VectorShuffle.vectorSpecies() => VectorShuffle.species()  (as in Vector and VectorMask)

Ok.


> - VectorShuffle.toVector() should return a IntVector

No, it returns a vector according to its species.  Note the constraint that shuffle source indexes are always in the range of -VLENGTH to VLENGTH - 1.


> - VectorShuffle.laneIsValid(), it seems to be an operation on Vector, not VectorShuffle.
> 

No, its specific to shuffle for masking out exceptional indexes (those < 0).


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

It's to reduce the number of methods. On Vector:

* This is not a full-service named operation like
* {@link #add(Vector) add()}.  A masked version of
* this operation is not directly available
* but may be obtained via the masked version of
* {@code lanewise}.  Subclasses define an additional
* scalar-broadcast overloading of this method.


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

Can you provide more details?

Thanks,
Paul.