[vector] RFC: Add scalable shapes for Arm Scalable Vector Extension
Ningsheng Jian (Arm Technology China)
Ningsheng.Jian at arm.com
Fri Nov 9 10:45:29 UTC 2018
Hi Vladimir,
I have updated the patch set based on your comments.
1) The VectorIntrinsics changes for reinterpret. Just rebase to the latest code, no changes compared to previous version.
http://cr.openjdk.java.net/~njian/vectorapi/01-vectorintrinsics-reinterpret.webrev2/
2) Adding Max vector shapes.
http://cr.openjdk.java.net/~njian/vectorapi/02-max-vector.webrev2/
Note: I have to keep the checks below to make unit tests passed, since current tests have to access and create *MaxVector. (We use MethodHandle in the tests to access internal hidden type, instead of preferredSpecies(). To make the tests passed on existing machines, we need to keep the checks to avoid aliasing.)
- } else if (o.bitSize() == 256) {
+ } else if (o.bitSize() == 256 && (o instanceof $Type$256Vector)) {
3) Adding tests for Max vector shapes, with some minor code styles fixed (trailing spaces, indentation, etc.)
http://cr.openjdk.java.net/~njian/vectorapi/03-tests-max-vector.webrev2/
test/jdk/jdk/incubator/vector tests passed with and without VectorApiIntrinsics on our x86 machines and also passed on AArch64 machines without VectorApiIntrinsics.
Could you please help to take a look?
Thanks,
Ningsheng
> -----Original Message-----
> From: Vladimir Ivanov <vladimir.x.ivanov at oracle.com>
> Sent: Friday, November 2, 2018 6:17 AM
> To: Ningsheng Jian (Arm Technology China) <Ningsheng.Jian at arm.com>;
> panama-dev at openjdk.java.net
> Cc: nd <nd at arm.com>
> Subject: Re: [vector] RFC: Add scalable shapes for Arm Scalable Vector Extension
>
> Ningsheng,
>
> >> First of all, looking at the changes in isolation, I think it's the
> >> right time to choose a better name. The usages are scattered around
> >> the code base and it'll be harder to change it later. As I mentioned
> >> earlier, IMO "Scalable" term doesn't communicate the intent clearly enough.
> >>
> >> From VM perspective with its Matcher::max_vector_size(),
> >> [Int|...]*Max*Vector looks natural. On JDK level,
> >> [Int|...]*Preferred*Vector looks a bit more favorable considering there's
> Vector.preferredSpecies().
> >>
> >> As for me, [Int|...]MaxVector looks better.
> >>
> >
> > OK, I can update the name in next webrev. If anyone else has other
> > better name, I am open to change. :-)
>
> Thanks!
>
> >> src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Shapes.java:
> >>
> >> + public static final SScalableBit S_Scalable_BIT = new SScalableBit();
> >> + public static final class SScalableBit extends Vector.Shape {
> >>
> >> I don't think there's a need in additional Shape instantiation. It
> >> conflicts with
> >> Vector.preferredSpecies() and current API should already support
> >> shape-agnostic usages.
> >>
> >> Also, its size may depend on element type and
> >> Vector.preferredSpecies() handles that.
> >>
> >> Am I missing any pros of SScalableBit compared to Vector.preferredSpecies()?
> >>
> >
> > My intention is to have a shape to represent any shapes other than the known
> explicit shapes (64/128/256/512). If there's no SScalableBit, I don't know what
> Vector.shapeForVectorBitSize() should return for any other machine supported
> max vector bits, like 768, 1024, 2048 etc. And treat it as the same level of other
> explicit shapes makes implementation easier. Perhaps, we can make them
> (SScalableBit and S_Scalable_BIT) non-public?
> >
>
> Ok, now I see where it is needed. Let's hide it for now then.
>
> >>
> >> src/jdk.incubator.vector/share/classes/jdk/incubator/vector/X-
> >> VectorBits.java.template:
> >>
> >> - if (o.bitSize() == 64) {
> >> + if ((o.bitSize() == 64) && (o instanceof
> >> + $Type$64Vector)) {
> >>
> >> If SScalableBit is gone, any other use cases for that checks except
> >> testing? From Alejandro comment [2], I got an impression that
> >> explicitly sized variants should be preferred to new variants. In
> >> that case, I don't see a way for vectors of equivalent (but different) shapes to
> meet at runtime.
> >>
> >
> > Yes, if SScalableBit is gone or hidden, it is impossible to have such conflict.
>
> So, if it's hidden those additional checks can go away (depending on unit test
> results), right?
>
> Best regards,
> Vladimir Ivanov
>
> >>
> >>>> -----Original Message-----
> >>>> From: Vladimir Ivanov <vladimir.x.ivanov at oracle.com>
> >>>> Sent: Wednesday, October 31, 2018 1:40 AM
> >>>> To: Ningsheng Jian (Arm Technology China) <Ningsheng.Jian at arm.com>;
> >>>> panama-dev at openjdk.java.net
> >>>> Cc: nd <nd at arm.com>
> >>>> Subject: Re: [vector] RFC: Add scalable shapes for Arm Scalable
> >>>> Vector Extension
> >>>>
> >>>> Thanks, Ningsheng.
> >>>>
> >>>>> I have separated vectorapi.webrev01 into three patches. Now they are:
> >>>>>
> >>>>> The scalable vector related changes:
> >>>>> http://cr.openjdk.java.net/~njian/vectorapi/01-scalable-vector.web
> >>>>> re
> >>>>> v0
> >>>>> /
> >>>>>
> >>>>> Vector intrinsics related changes:
> >>>>> http://cr.openjdk.java.net/~njian/vectorapi/02-vector-intrinsics.w
> >>>>> eb
> >>>>> re
> >>>>> v0/
> >>>>
> >>>> What I had in mind is getting VectorIntrinsics.reinterpret/cast()
> >>>> and all related changes (jdk + hotspot) separated, to focus on
> >>>> *ScalableVector shapes in a separate webrev.
> >>>>
> >>>> Sorry for the confusion.
> >>>>
> >>>>> AArch64 specific changes:
> >>>>> http://cr.openjdk.java.net/~njian/vectorapi/03-aarch64-sve.webrev0
> >>>>> /
> >>>>
> >>>> Looks fine. It looks like a stand-alone change ready to be pushed.
> >>>> Do you
> >> agree?
> >>>>
> >>>> Best regards,
> >>>> Vladimir Ivanov
> >>>>
> >>>>>
> >>>>> And the tests (the same as previous version):
> >>>>> http://cr.openjdk.java.net/~njian/vectorapi/04-tests-scalable-vect
> >>>>> or
> >>>>> .w
> >>>>> ebrev0/
> >>>>>
> >>>>> Could you please help to take a look?
> >>>>>
> >>>>> Thanks,
> >>>>> Ningsheng
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Halimi, Jean-Philippe <jean-philippe.halimi at intel.com>
> >>>>>> Sent: Tuesday, October 30, 2018 8:55 AM
> >>>>>> To: Vladimir Ivanov <vladimir.x.ivanov at oracle.com>; Ningsheng
> >>>>>> Jian (Arm Technology China) <Ningsheng.Jian at arm.com>;
> >>>>>> panama-dev at openjdk.java.net
> >>>>>> Cc: nd <nd at arm.com>
> >>>>>> Subject: RE: [vector] RFC: Add scalable shapes for Arm Scalable
> >>>>>> Vector Extension
> >>>>>>
> >>>>>> I do not have more comments, tests look good to me.
> >>>>>>
> >>>>>> Jp
> >>>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Vladimir Ivanov [mailto:vladimir.x.ivanov at oracle.com]
> >>>>>> Sent: Monday, October 29, 2018 4:16 PM
> >>>>>> To: Ningsheng Jian (Arm Technology China)
> >>>>>> <Ningsheng.Jian at arm.com>; panama-dev at openjdk.java.net; Halimi,
> >>>>>> Jean-Philippe <jean- philippe.halimi at intel.com>
> >>>>>> Cc: nd <nd at arm.com>
> >>>>>> Subject: Re: [vector] RFC: Add scalable shapes for Arm Scalable
> >>>>>> Vector Extension
> >>>>>>
> >>>>>> Ningsheng,
> >>>>>>
> >>>>>>> http://cr.openjdk.java.net/~njian/sve/vectorapi.webrev01/
> >>>>>>> and the test cases:
> >>>>>>> http://cr.openjdk.java.net/~njian/sve/vectorapi-test.webrev01/
> >>>>>>>
> >>>>>>> All the vector api cases passed on my x86 machines with and
> >>>>>>> without
> >>>>>> VectorApiIntrinsics. There are some failures on AArch64, but they
> >>>>>> are not related to this patch and we will fix them in follow-up patches.
> >>>>>>
> >>>>>> vectorapi-test.webrev01 looks good. Jp, do you have any comments?
> >>>>>>
> >>>>>> Regarding vectorapi.webrev01, please, separate it in 3 patches:
> >>>>>> (1) VectorIntrinsics-related changes
> >>>>>> (2) ScalableVector-related changes
> >>>>>> (3) aarch64-specific changes
> >>>>>>
> >>>>>> IMO #3 is fine, but I'd like to hear from Intel folks about #1 & #2.
> >>>>>>
> >>>>>> Best regards,
> >>>>>> Vladimir Ivanov
> >>>>>>
> >>>>>>>
> >>>>>>> Please help to review the changes.
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>> Ningsheng
> >>>>>>>
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Vladimir Ivanov <vladimir.x.ivanov at oracle.com>
> >>>>>>>> Sent: Friday, October 12, 2018 11:18 AM
> >>>>>>>> To: Ningsheng Jian (Arm Technology China)
> >>>>>>>> <Ningsheng.Jian at arm.com>; panama-dev at openjdk.java.net
> >>>>>>>> Cc: nd <nd at arm.com>
> >>>>>>>> Subject: Re: [vector] RFC: Add scalable shapes for Arm Scalable
> >>>>>>>> Vector Extension
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>>>> So, my question boils down to: how vector shape checks are
> >>>>>>>>>>>> expected to work on *ScalableVectors in presence of
> >>>>>>>>>>>> operations which change vector
> >>>>>>>>>> size.
> >>>>>>>>>>>>
> >>>>>>>>>>>> For example, IntScalableVector.rebracket() =>
> >>>>>>>>>>>> LongScalableVector which is fine, but ISV.resize() => ISV
> >>>>>>>>>>>> is what I'm concerned about and existing checks aren't
> >>>>>>>>>>>> enough for *ScalableVectors unless you check their length at
> runtime.
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks for the detailed explanation. So, which checks do you
> >>>>>>>>>>> mean here? Do
> >>>>>>>>>> you mean this [1]? I find that current Java implementations
> >>>>>>>>>> already has some runtime length checks. One thing we need to
> >>>>>>>>>> consider is the ambiguity between existing sizes and scalable
> >>>>>>>>>> size. Current
> >>>>>>>>>> VectorIntrinsics.reinterpret() just tries to get the shape
> >>>>>>>>>> from size
> >>>>>>>>>> (library_call.cpp:get_exact_klass_for_vector_box) instead of
> >>>>>>>>>> getting the real class from input argument, which will lead
> >>>>>>>>>> to ClassCastException. Maybe that can be fixed by adding real
> >>>>>>>>>> klass type to the
> >>>>>>>> reinterpret function.
> >>>>>>>>>>
> >>>>>>>>>> The checks I referred to are the casts on vector shapes which
> >>>>>>>>>> are part of every vector operation (e.g., [2]), but they
> >>>>>>>>>> don't immediately relate to shape-changing operations.
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> OK, but I would expect the normal operations between SV and
> >>>>>>>>> other vectors
> >>>>>>>> are illegal, though they may have the same size, since users
> >>>>>>>> usually don't know the real size of SV. We should treat SV
> >>>>>>>> different from other
> >>>>>> vectors.
> >>>>>>>>
> >>>>>>>> Yes, the checks currently in place effectively forbid mixing
> >>>>>>>> vectors of different shapes. But API allows to reshape/cast
> >>>>>>>> vectors to arbitrary shapes and that may cause problems if
> >>>>>>>> equivalent shapes meet at
> >>>>>> runtime.
> >>>>>>>>
> >>>>>>>> Since concrete vector shapes aren't part of API, I believe it
> >>>>>>>> is possible to exclude duplicating shapes at runtime. For
> >>>>>>>> example, SVs can be used as a substitute for *512V shapes on
> >>>>>>>> 512-bit SVE or AVX512-capable hardware. It seems current JDK-
> >>>>>>>> JVM interface
> >>>>>>>> (VectorIntrinsics) should fit SV purposes well (both on x86 &
> >>>>>>>> ARM), but it would be very interesting to hear your experience.
> >>>>>>>>
> >>>>>>>> API allows to query vectors of smaller/larger sizes than
> >>>>>>>> hardware natively supports and implementation has to support
> >>>>>>>> that (even if
> >>>>>> performance suffers).
> >>>>>>>> It means SV shapes can coexist at runtime with existing shapes
> >>>>>>>> (*64V/.../*512V) and casts/reshapes between all those shapes
> >>>>>>>> should
> >> work.
> >>>>>>>>
> >>>>>>>> Best regards,
> >>>>>>>> Vladimir Ivanov
> >>>>>>>>
> >>>>>>>>>> It seems I had some wrong assumptions when coming up with the
> >>>> examples.
> >>>>>>>>>> Sorry for the confusion. Taking those into account, here's my
> >>>>>>>>>> take on current state of vector shape changing operations
> >>>>>>>>>> w.r.t. SV
> >>>> shapes.
> >>>>>>>>>>
> >>>>>>>>>> There'll be 30 vector shapes in total (5 sizes x 6 element types):
> >>>>>>>>>> * Byte64V, Byte128V, Byte256V, Byte512V, ByteSV
> >>>>>>>>>> * Short64V, ..., ShortSV
> >>>>>>>>>> ...
> >>>>>>>>>> * Double64V, Double128V, ... Double512V, DoubleSV
> >>>>>>>>>>
> >>>>>>>>>> Depending on hardware, SVs may alias with "explicitly sized"
> >>>>>>>>>> shapes (e.g., IntSV is equivalent to Int512V on AVX512 capable H/W).
> >>>>>>>>>>
> >>>>>>>>>> (1) Vector.rebracket(): works fine for SVs since vector size
> >>>>>>>>>> (in
> >>>>>>>>>> bits) stays the
> >>>>>>>> same:
> >>>>>>>>>> IntSV <-> LongSV <-> ... <-> DoubleSV.
> >>>>>>>>>>
> >>>>>>>>>> (2) Vector.resize(): SV->SV doesn't make sense and the
> >>>>>>>>>> operation should involve both SV and existing vector shapes:
> >>>>>>>>>> IntSV <-> Int64V/Int128V/.../Int512V. The ambiguity you
> >>>>>>>>>> mention can cause problems and needs to be carefully handled
> >>>>>>>>>> to avoid 2 equivalent shapes to meet at runtime. Otherwise,
> >>>>>>>>>> vector shape checks I mentioned
> >>>>>>>> earlier should be changed.
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Yes, we will try to address this issue carefully in the follow-up webrev.
> >>>>>>>>>
> >>>>>>>>>> (3) Vector.cast(): SV->SV, SV<->64V/.../512V
> >>>>>>>>>> * size-preserving transformations (int <-> float):
> >>>>>>>>>> SV->SV work fine: IntSV <-> FloatSV, LongSV <->
> >>>>>>>>>> DoubleSV;
> >>>>>>>>>>
> >>>>>>>>>> * widening casts (e.g., int->long)
> >>>>>>>>>> SV->SV/64V/...: truncate upper part of the
> >>>>>>>>>> vector, since SVs already represent widest vector shapes;
> >>>>>>>>>> 64V/...->SV: either truncated or filled with
> >>>>>>>>>> defaults depending on actual size
> >>>>>>>>>>
> >>>>>>>>>> * narrowing casts (e.g., long->int)
> >>>>>>>>>> SV->SV/64V/...: fill upper part of SV with defaults;
> >>>>>>>>>> 64V/...->SV: either truncated or filled with
> >>>>>>>>>> defaults depending on actual size
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Thanks,
> >>>>>>>>> Ningsheng
> >>>>>>>>>
More information about the panama-dev
mailing list