[vector api] Vector elements shift confusion

[Bhateja, Jatin]

Hi All,

Following is the link to changes.

http://cr.openjdk.java.net/~vdeshpande/Elemental_Rotate/webrev.00/

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