FW: [vector api] Vector elements shift confusion
Bhateja, Jatin
jatin.bhateja at intel.com
Thu Feb 28 19:24:45 UTC 2019
Hi Vladimir,
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/
Kindly review.
Thanks,
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
jshell> V256.shiftEL(2)
$26 ==> [0.0, 0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0]
jshell> V256.shiftER(2)
$27 ==> [3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 0.0, 0.0]
jshell> V256.rotateEL(2)
$28 ==> [7.0, 8.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0]
jshell> V256.rotateER(26)
$29 ==> [3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 1.0, 2.0]
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