[vector] RFC: Add scalable shapes for Arm Scalable Vector Extension
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Sat Nov 10 01:42:25 UTC 2018
> 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)) {
Ok, let's leave it as is for now.
Pushed all 3 changes [1] [2]. Nice work!
Best regards,
Vladimir Ivanov
[1] http://hg.openjdk.java.net/panama/dev/rev/c31067b79f47
[2] http://hg.openjdk.java.net/panama/dev/rev/5dae74e21bd5
>
> 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