[vector] RFC: Add scalable shapes for Arm Scalable Vector Extension
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Wed Oct 31 20:51:20 UTC 2018
Ningsheng,
> 2) Scalable vector related changes:
> http://cr.openjdk.java.net/~njian/vectorapi/02-scalable-vector.webrev1/
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.
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()?
There's a need to access new shapes in unit tests, but it doesn't
require public API and can be implemented differently (e.g., by module
patching, see MethodHandleHelper [1] for an example).
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.
> 4) Tests (the same as previous version):
> http://cr.openjdk.java.net/~njian/vectorapi/04-tests-scalable-vector.webrev1/
Best regards,
Vladimir Ivanov
[1]
http://hg.openjdk.java.net/panama/dev/file/aa68196d5ef3/test/jdk/java/lang/invoke/java.base/java/lang/invoke/MethodHandleHelper.java
[2]
http://mail.openjdk.java.net/pipermail/panama-dev/2018-October/002967.html
>> -----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.webrev0
>>> /
>>>
>>> Vector intrinsics related changes:
>>> http://cr.openjdk.java.net/~njian/vectorapi/02-vector-intrinsics.webre
>>> 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-vector.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