[vector] RFC: Add scalable shapes for Arm Scalable Vector Extension
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Thu Nov 1 22:16:32 UTC 2018
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.webre
>>>>> v0
>>>>> /
>>>>>
>>>>> Vector intrinsics related changes:
>>>>> http://cr.openjdk.java.net/~njian/vectorapi/02-vector-intrinsics.web
>>>>> 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-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