[vector api] Vector elements shift confusion

[Vladimir Ivanov]

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