FW: [vector api] Vector elements shift confusion

Thu Feb 28 20:36:53 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!

> jshell> var F256 =
> jshell> FloatVector.species(jdk.incubator.vector.FloatVector.Shape.S_256
> jshell> _BIT);
> F256 ==> Shape[256 bits, 8 floats x 32 bits]
> 
> jshell> var V256 = F256.fromArray(new float[]
> jshell> {1.0f,2.0f,3.0f,4.0f,5.0f,6.0f,7.0f,8.0f},0);
> V256 ==> [1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0]
> 
> jshell> V256.get(0)
> $24 ==> 1.0
> 
> jshell> V256.get(7)
> $25 ==> 8.0

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

> 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
>>>>>>>