FW: [vector api] Vector elements shift confusion
Bhateja, Jatin
jatin.bhateja at intel.com
Fri Mar 1 04:22:14 UTC 2019
Hi Vladimir,
Few queries embedded in below mail.
Thanks,
Jatin
> -----Original Message-----
> From: Vladimir Ivanov [mailto:vladimir.x.ivanov at oracle.com]
> Sent: Friday, March 1, 2019 2:07 AM
> To: Bhateja, Jatin <jatin.bhateja at intel.com>
> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>;
> tomasz.kowalczewski at gmail.com; panama-dev at openjdk.java.net
> Subject: Re: FW: [vector api] Vector elements shift confusion
>
>
>
> > 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!
If no more comments over patch can you kindly push it to branch.
>
> > jshell> var F256 =
> > jshell> FloatVector.species(jdk.incubator.vector.FloatVector.Shape.S_2
> > jshell> 56
> > 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
>
Cannot follow your above comment.
> > 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
> >>>>>>>
More information about the panama-dev
mailing list