Vector API review

[forax at univ-mlv.fr]

----- Mail original -----
> De: "John Rose" <john.r.rose at oracle.com>
> À: "Remi Forax" <forax at univ-mlv.fr>
> Cc: "panama-dev at openjdk.java.net'" <panama-dev at openjdk.java.net>
> Envoyé: Jeudi 1 Avril 2021 01:48:51
> Objet: Re: Vector API review

> Thanks for the read-over and the careful review.
> 
> I disagree with most of your suggestions, even though
> they are reasonable.  This is a good moment to explain
> why some things are the way they are.

I think it's Ok to disagree.

> 
> On Mar 31, 2021, at 2:43 PM, Remi Forax <forax at univ-mlv.fr> wrote:
>> 
>> Hi all,
>> next week i will do a lab on the vector API, so i've played a little more with
>> the API.
>> 
>> There are still things to polish,
>> 
>> - IntVector.addIndex() should be addLaneIndex(int scale)
> 
> Good.
> 
>> - Vector.lanewise should be Vector.apply given that test or conversion are also
>> lanewise,
>>  calling a function is usually "apply".
> 
> (Already considered and rejected.)
> 
> The most important fact is not that the op is a function,
> but rather that the vector has lanes.  Think of “lanewise”
> as a graceful abbreviation of “applyLanewise”.

yep, the issue is that test is a short name for testLanewise, convert is the short name for convertLanewise, but lanewise is the short name for applyLanewise.

> 
>> - While Vector.lanewise is overloaded for unary, binary, ternary and
>> associative,
>>  for unary test and binary test, we have two different name test and compare.
>>  I think, compare should be renamed to test.
>>  And VectorOperators.Test should be VectorOperators.UnaryTest and Comparison
>>  should be BinaryTest.
> 
> (Already considered and rejected.)
> 
> You test one thing and you compare two things.

yes, but grouping overloads is interesting because it means less concept to understand.
If we group the apply/lanewise, we should group the others, hence renaming test to compare or vice-versa.

Is comparing to a constant, a kind of unary comparison not something acceptable.

> 
> In this case it also could have been called lanewise, but
> making it return a VectorMask would have required tricky
> overload stuff.

yes,

[...]

> 
>>  In term of javadoc, all the interfaces like VectorOperations.Unary, Binary, etc
>>  should document all the possible operations,
>>  instead we have to crawle VectorOperations looking for the right type.
> 
> Maybe.  Although the IDE will do that for you during completion,
> because of the typefulness of that argument position.

nope, an IDE can not do that because you can add a dot after the constant.
By example if you have
  reduceLane(SUB
is ok because you can do something like
  reduceLane(SUB.giveMeTheOpposite());

> 
>>  There is no iota as a static method on IntVector, LongVector, etc, so we have to
>>  use zero().addIndex(1) which is weird
>>  (or i've missed something ?)
> 
> No, it’s just an arbitrary choice to leave the user to compose
> addIndex with a constant zero.
> 
>> - Having add, sub, etc on Vector is useless given that we already have
>> lanewise().
> 
> That’s a conscious decision, documented in the package-info.
> The universal API is lanewise (and test/compare), but the basic
> full-support operations *also* have named methods.  Surely
> you can see this is designed to be pedagogically helpful.
> Not useless at all, just not functionally minimal.

Pedagogically, you will use an IntVector not a Vector.

> 
>>  Having add, etc, etc on the specialized version, IntVector, LongVector, is a
>>  better idea because the parameter type can be typed
>>  with the right specialized type instead of Vector, e.g, IntVector.add(IntVector)
>>  instead of InteVector.add(Vector)
> 
> (That’s true in general of lanewise also.)

You need lanewise() on Vector for the case somebody want to work on an abstract Vector and is foolish enough to think that the JIT will see through and de-virtualize the call.
But having add() on Vector, i.e. having a user friendly method on a non user friendly type tries to serve two masters at the same time.

> 
>> - Can wa agree that having a static method and an instance method with the same
>> name like broadcast is not a good idea
> 
> In general, yes.  But broadcast is very special.
> 
>>  I don't see the point of the instance method given that it replaces all the
>>  values.
> 
> This is the point:
> 
> var v = (something configuration sensitive);
> var w = v.broadcast(42);
> (do stuff with v and w)
> 
> You need a graceful way to break up stuff like this:
> 
> v.lanewise(op, 42);
> 
> into stuff like this:
> 
> var k42 = v.broadcast(42);
> …more stuff added...
> v.lanewise(op, k42);
> 
> We must not assume that the static type of v is
> always readily available, to invoke a static factory.

This is maybe a valid point as an implementer of the API, less as a user of that API given that you already have overloads like v.lanewise(op, 42) in the API.
The other issue is more fundamental, to generate the vector code the VM needs to know the vector element type and the vector shape.
The vector element type usually comes from the specialized Vector, IntVector, ByteVector etc and the vector shape comes from the VectorSpecies so it has to be a constant to the JIT.
That why all creational/factory methods that returns a Vector takes a VectorSpecies as parameter.
So to get good performance when you create a vector, you have to pass the SPECIES, using all other method may fail, calling the instance broadcast on an IntVector may fail to be vectorized because the JIT has lost the vector shape of this IntVector (due to profile pollution). Using a static method like the other broadcast that takes a SPECIES has not this issue.
I think the API should avoid to provide instance methods calling a static factories for that reason.
And to be perfectly clear, i see no problem to have this method in the implementations because in your implementation you know what you are doing, but the bar to have for such methods in the public API is higher IMO. 

> 
>> - I also don't see the point of maskAll() given that it's species().maskAll()
> 
> Pedagogy.  But I agree it’s a slippery slope.

and see above

> 
>> - I don't see why convertShape is not named convert too given that convert also
>> returns a vector with a different shape
> 
> The conversions are different enough that they need
> different names to avoid confusion.  Shape changes
> are tricky and may have extra cost, so need to be
> called out explicitly.

Ok,

> 
> I don’t see how plain convert changes shape…?

My bad, it changes the Species but not the shape.

> 
>> - remove selectFrom() given that it's rearrange()
> 
> We added that for the about same reason some ISAs
> have both SUB and RSUB.  There are use cases for
> either, and it’s artificial to require minimality here.

in that case, add overloads to rearrange(), because it takes me 5 mins to figure out that selectFrom was rearrange.
Again, it goes with the idea that limiting the number of verbs in a complex API like this one is a good idea, it means fewer concept to understand.

> 
>> - I dont see the point of the reinterpretAsFoo, perhaps a
>> reinterpretAs(Class<?>) is enough ?
> 
> No, it’s not.  The static return type is important, and
> cannot be derived polymorphically from a Class arg.

Even if the class arg is the array (like for asCollector in the MethodHandles API) ?

> 
>> 
>> - check() can renamed to "is"/"isInstance" because it's an instanceof test
> 
> Not within the vector paradigm.  Tests are lanewise and generate masks.
> If you really want an instanceof test, use the instanceof operator,
> but that isn’t part of the vector programming model.
> 
> In any case, it’s a cast not a test.  It is sometimes useful to re-derive
> static type information (in fluent syntax), after an API point has lost
> that information.

I get that it's either an element type check or a species check, but the name "check" is usually used with methods that return void.
Perhaps "as" or "requires" are better names.
BTW, why IntVector.checks() doesn't return an IntVector ?? 

> 
> — John

Rémi