[vector api] Vector elements shift confusion

[Viswanathan, Sandhya]

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