FW: [vector api] Vector elements shift confusion
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Fri Mar 1 21:48:24 UTC 2019
>>> I used jshell to explore the working of current elemental shift and rotate
>> APIs and there was a disconnect / confusion b/w API implementation vs
>> docs.
>>> So following is the behavior with this patch, I guess it's more intuitive and in
>> accordance with what you suggested.
>>> http://cr.openjdk.java.net/~kkharbas/ElementalRotate/webrev.01/
>>
>> Looks good, Jatin!
> If no more comments over patch can you kindly push it to branch.
FTR no need to rely on me to push patches. Some of your colleagues at
Intel have Committer rights and can push it for you until you become
Committer yourself.
I went forward with the push, but spotted a test failure during a test run:
FAILED: jdk/incubator/vector/VectorHash.java
The following patch [1] fixes the failure, but there are other
occurences in benchmarks [2]. Please, fix those as well.
[1]
diff --git a/test/jdk/jdk/incubator/vector/VectorHash.java
b/test/jdk/jdk/incubator/vector/VectorHash.java
--- a/test/jdk/jdk/incubator/vector/VectorHash.java
+++ b/test/jdk/jdk/incubator/vector/VectorHash.java
@@ -173,7 +173,7 @@
h = h * top_h_coeff + x.mul(v_h_coeff).addAll();
- b = b.shiftEL(intSpecies.length());
+ b = b.shiftER(intSpecies.length());
}
}
[2]
test/jdk/jdk/incubator/vector/benchmark/src//main/java/benchmark/jdk/incubator/vector/AbstractVectorBenchmark.java:
var v2 = hi.reshape(to).shiftER(vlen);
test/jdk/jdk/incubator/vector/benchmark/src//main/java/benchmark/jdk/incubator/vector/AbstractVectorBenchmark.java:
var v3 = v2.shiftER(i * vlen); // [0 0 ... 0 | 1 1 ... 1 | 0 0
... 0]
test/jdk/jdk/incubator/vector/benchmark/src//main/java/benchmark/jdk/incubator/vector/AbstractVectorBenchmark.java:
var vb = ((IntVector)(va.shiftEL(k *
B64.length()).reshape(B64).cast(species))).and(0xFF);
test/jdk/jdk/incubator/vector/benchmark/src//main/java/benchmark/jdk/incubator/vector/SumOfUnsignedBytes.java:
var accHi =
((IntVector)(acc.shiftEL(mid).reshape(S128).cast(I256))).and(0xFFFF); //
high half as ints
...
>> Vector.toString() implementation doesn't help with making the behavior
>> clearer :-) It's worth to consider improving it as well at some point.
>>
>> Also, a paragraph in general API description describing the difference
>> between memory layout (little-endian) & in-register representation w.r.t.
>> cross-lane operations may help.
>>
>> But that's for follow-up work.
>>
>>> jshell> V256.shiftEL(2)
>>> $26 ==> [0.0, 0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0]
>>
>> "left" == 0 =2=> 7
>>
> Cannot follow your above comment.
Just wanted to illustrate how confusing is Vector.toString() output
w.r.t. shiftE*/rotateE*: the vector elements "move" right while the
operation has "L" in its name :-)
Best regards,
Vladimir Ivanov
>>> jshell> V256.shiftER(2)
>>> $27 ==> [3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 0.0, 0.0]
>>
>> "right" == 0 <=2= 7
>>
>>> jshell> V256.rotateEL(2)
>>> $28 ==> [7.0, 8.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0]
>>
>> "left" == 0 =2=> 7
>>>
>>> jshell> V256.rotateER(26)
>>> $29 ==> [3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 1.0, 2.0]
>>
>> "right" == 0 <=2= 7
>>
>> Best regards,
>> Vladimir Ivanov
>>
>>> jshell> V256.shiftEL(2)
>>> $30 ==> [0.0, 0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0]
>>>
>>> jshell> V256.shiftEL(2).get(0)
>>> $31 ==> 0.0
>>>
>>> jshell> V256.shiftEL(2).get(2)
>>> $32 ==> 1.0
>>>
>>> jshell> V256.shiftER(2).get(7)
>>> $33 ==> 0.0
>>>
>>> jshell> V256.shiftER(2).get(5)
>>> $34 ==> 8.0
>>>
>>>> -----Original Message-----
>>>> From: Vladimir Ivanov [mailto:vladimir.x.ivanov at oracle.com]
>>>> Sent: Thursday, February 28, 2019 10:51 AM
>>>> To: Bhateja, Jatin <jatin.bhateja at intel.com>; Viswanathan, Sandhya
>>>> <sandhya.viswanathan at intel.com>; Tomasz Kowalczewski
>>>> <tomasz.kowalczewski at gmail.com>
>>>> Cc: panama-dev at openjdk.java.net
>>>> Subject: Re: [vector api] Vector elements shift confusion
>>>>
>>>> Hi Jatin,
>>>>
>>>> Thanks for taking care of the issue and welcome to the project!
>>>>
>>>>> http://cr.openjdk.java.net/~vdeshpande/Elemental_Rotate/webrev.00/
>>>> Can you, please, elaborate why do you switch rotateER & rotateEL
>>>> meaning instead of shiftER & shiftEL.
>>>>
>>>> There's already enough confusion about "left" & "right" in the API
>>>> and I'm all for keeping it as uniform as possible.
>>>>
>>>> If we agree that EL means moving elements in index increasing
>>>> direction (from lower to higher) and ER moves them in the opposite
>>>> direction (from higher to lower), that (sort of) aligns the behavior
>>>> with shiftL/shiftR/aShiftR (but on a bit level).
>>>>
>>>> The fact that XxxVector.fromArray()/intoArray specify little-endian
>>>> layout [1] complicates things even more, but IMO aligning behavior
>>>> with >>/>>>/<< operations doesn't make it worse (the situation is very
>> similar for primitives).
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>>>
>>>> [1]
>>>> /**
>>>> * Loads a vector from an array starting at offset.
>>>> * <p>
>>>> * For each vector lane, where {@code N} is the vector lane index, the
>>>> * array element at index {@code i + N} is placed into the
>>>> * resulting vector at lane index {@code N}.
>>>>
>>>>
>>>>
>>>>>
>>>>> Kindly review and push it through.
>>>>>
>>>>> Thanks,
>>>>> Jatin
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Viswanathan, Sandhya
>>>>>> Sent: Wednesday, January 23, 2019 11:59 PM
>>>>>> To: Vladimir Ivanov <vladimir.x.ivanov at oracle.com>; Tomasz
>>>>>> Kowalczewski <tomasz.kowalczewski at gmail.com>
>>>>>> Cc: panama-dev at openjdk.java.net; Bhateja, Jatin
>>>>>> <jatin.bhateja at intel.com>
>>>>>> Subject: RE: [vector api] Vector elements shift confusion
>>>>>>
>>>>>> Hi Vladimir,
>>>>>>
>>>>>>>> At some point, rotateER/L and shiftER/L should be intrinsified,
>>>>>>>> but
>>>>>> unfortunately they haven't got enough attention yet. Sandhya, what
>>>>>> are the plans on that front? Does anybody working on it?
>>>>>>
>>>>>> Yes we plan to take it up in February (Jatin cced here) after the
>>>>>> min/max/minAll/maxAll fix to match the new specification.
>>>>>>
>>>>>> Best Regards,
>>>>>> Sandhya
>>>>>>
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Vladimir Ivanov [mailto:vladimir.x.ivanov at oracle.com]
>>>>>> Sent: Wednesday, January 23, 2019 10:24 AM
>>>>>> To: Tomasz Kowalczewski <tomasz.kowalczewski at gmail.com>;
>>>> Viswanathan,
>>>>>> Sandhya <sandhya.viswanathan at intel.com>
>>>>>> Cc: panama-dev at openjdk.java.net
>>>>>> Subject: Re: [vector api] Vector elements shift confusion
>>>>>>
>>>>>>
>>>>>>> Thanks Vladimir for taking time to look into it. If it makes sense
>>>>>>> I can try to fix these (should be simple - on Java level) and post
>>>>>>> a webrev (I have signed the OCA).
>>>>>>
>>>>>> Sure, contributions are more than welcome!
>>>>>>
>>>>>> At some point, rotateER/L and shiftER/L should be intrinsified, but
>>>>>> unfortunately they haven't got enough attention yet. Sandhya, what
>>>>>> are the plans on that front? Does anybody working on it?
>>>>>>
>>>>>> Best regards,
>>>>>> Vladimir Ivanov
>>>>>>
>>>>>>> On Wed, Jan 23, 2019 at 4:12 AM Vladimir Ivanov
>>>>>>> <vladimir.x.ivanov at oracle.com> wrote:
>>>>>>>>
>>>>>>>> Good catch, Tomasz! I confirm that rotateER is completely broken
>>>>>>>> :-)
>>>>>>>>
>>>>>>>> There's definitely disagreement between javadoc & implementation
>>>>>>>> on shiftEL/ER variants, but I'm in favor of the javadoc here and
>>>>>>>> would like to see the implementation aligned with it.
>>>>>>>>
>>>>>>>> While fromArray() and [shift|rotate]EL/ER seem to disagree
>>>>>>>> (little-endian vs big-endian style), it aligns with the behavior
>>>>>>>> of shifts on primitives where:
>>>>>>>> "left" == "towards higher bits" == "towards higher indices"
>>>>>>>> "right" == "towards lower bits" == "towards higher indices"
>>>>>>>>
>>>>>>>> And as you noted, implementation behaves in the opposite
>>>>>>>> direction while rotateEL agrees with the spec [1].
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Vladimir Ivanov
>>>>>>>>
>>>>>>>> [1] $ jshell --add-modules=jdk.incubator.vector
>>>>>>>>
>>>>>>>> > import jdk.incubator.vector.*
>>>>>>>>
>>>>>>>> > var I256 =
>>>>>>>> IntVector.species(jdk.incubator.vector.Vector.Shape.S_256_BIT);
>>>>>>>>
>>>>>>>> > var v1 = I256.fromArray(new int[] { 1, 2, 3, 4, 5, 6, 7, 8
>>>>>>>> },
>>>>>>>> 0)
>>>>>>>> v1 ==> [1, 2, 3, 4, 5, 6, 7, 8]
>>>>>>>>
>>>>>>>> > v1.get(0)
>>>>>>>> ==> 1
>>>>>>>>
>>>>>>>> > v1.get(7)
>>>>>>>> ==> 8
>>>>>>>>
>>>>>>>> > v1.shiftEL(1)
>>>>>>>> ==> [2, 3, 4, 5, 6, 7, 8, 0]
>>>>>>>>
>>>>>>>> > v1.shiftEL(1).get(0)
>>>>>>>> ==> 2
>>>>>>>>
>>>>>>>> > v1.shiftEL(1).get(7)
>>>>>>>> ==> 0
>>>>>>>>
>>>>>>>> > v1.shiftER(1)
>>>>>>>> ==> [0, 1, 2, 3, 4, 5, 6, 7]
>>>>>>>>
>>>>>>>> > v1.shiftER(1).get(0)
>>>>>>>> ==> 0
>>>>>>>>
>>>>>>>> > v1.shiftER(1).get(7)
>>>>>>>> ==> 7
>>>>>>>>
>>>>>>>> > v1.rotateEL(1)
>>>>>>>> ==> [8, 1, 2, 3, 4, 5, 6, 7]
>>>>>>>>
>>>>>>>> jshell> v1.rotateEL(1).get(0)
>>>>>>>> ==> 8
>>>>>>>>
>>>>>>>> jshell> v1.rotateEL(1).get(7)
>>>>>>>> ==> 7
>>>>>>>>
>>>>>>>> > v1.rotateER(1)
>>>>>>>> | Exception java.lang.ArrayIndexOutOfBoundsException: Index -1
>>>>>>>> |out of
>>>>>>>> bounds for length 8
>>>>>>>> | at Int256Vector.rotateER (Int256Vector.java:924)
>>>>>>>> | at Int256Vector.rotateER (Int256Vector.java:39)
>>>>>>>> | at (#44:1)
>>>>>>>>
>>>>>>>>
>>>>>>>>> I started this email having problems using rotateER on vectors.
>>>>>>>>> I have not found any way to call this method without getting an
>>>>>>>>> exception
>>>>>> :).
>>>>>>>>> More investigation revealed that shift-ing vector elements does
>>>>>>>>> not work in accordance with javadoc. I realised this email will
>>>>>>>>> be quite long so I split it up and will describe just shift
>>>>>>>>> issues. If my suspicions are correct I will follow up with rotations.
>>>>>>>>>
>>>>>>>>> First shiftEL operation describes itself "as if rotating left
>>>>>>>>> the lane elements by i [...] zero value is placed into the
>>>>>>>>> result vector at lane indexes less than i % this.length()."
>>>>>>>>>
>>>>>>>>> *I suspect that the rotation is described in the wrong direction*.
>>>>>>>>> Lets try to confirm it but just looking at zeroed lane indices
>>>>>>>>> (using
>>>>>>>>> get() which "Gets the lane element at lane index i"):
>>>>>>>>>
>>>>>>>>> @Test
>>>>>>>>> public void shouldShiftElementsLeft() {
>>>>>>>>> // Given
>>>>>>>>> IntVector intVector =
>>>>>>>>> IntVector.species(Vector.Shape.S_128_BIT).fromArray(new int[]{
>>>>>>>>> 1, 2, 3, 4
>>>>>>>>> }, 0);
>>>>>>>>>
>>>>>>>>> // When
>>>>>>>>> intVector = intVector.shiftEL(2);
>>>>>>>>>
>>>>>>>>> // Then
>>>>>>>>> assertThat(intVector.get(0)).isEqualTo(0);
>>>>>>>>> assertThat(intVector.get(1)).isEqualTo(0);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> So after the shift of "2" lane indexes less than "2" should be zero.
>>>>>>>>> This test fails as the rotation is done in the opposite (to me
>>>>>>>>> -
>>>>>>>>> correct) direction:
>>>>>>>>>
>>>>>>>>> @Test
>>>>>>>>> public void shouldShiftElementsLeft() {
>>>>>>>>> // Given
>>>>>>>>> IntVector intVector =
>>>>>>>>> IntVector.species(Vector.Shape.S_128_BIT).fromArray(new int[]{
>>>>>>>>> 1, 2, 3, 4
>>>>>>>>> }, 0);
>>>>>>>>>
>>>>>>>>> // When
>>>>>>>>> intVector = intVector.shiftEL(2);
>>>>>>>>>
>>>>>>>>> // Then
>>>>>>>>> assertThat(intVector.toArray()).containsExactly(3, 4, 0,
>>>>>>>>> 0); }
>>>>>>>>>
>>>>>>>>> Same case is with shiftER. Java doc says that "zero value is
>>>>>>>>> placed into the result vector at lane indexes greater or equal
>>>>>>>>> to
>>>>>>>>> this.length() - (i % this.length()).". But zeroed are indexes <
>>>>>>>>> i % this.length().
>>>>>>>>>
>>>>>>>>> Looks like implementations are correct but javadoc-s are swapped
>>>>>>>>> (I hope I am not making fool of myself).
>>>>>>>>>
>>>>>>>>> Please advise,
>>>>>>>>> Tomasz
>>>>>>>>>
More information about the panama-dev
mailing list