Vector API review

[August Nagro]

Concerning Vector<Integer> vs IntVector, it would be nice to have a
VectorMask::to vector overload that returns specialized versions.

Otherwise you often end up with the less featureful Vector<?> generic
class. For example,

Vector<Int> res = myIntVector.eq(anotherVector).toVector()

You can reinterpretAsInts() to get back to IntVector, but that entails a
copy?




On Thu, Apr 1, 2021, 8:38 AM <forax at univ-mlv.fr> wrote:

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