FW: [vector api] Vector elements shift confusion
Bhateja, Jatin
jatin.bhateja at intel.com
Wed Mar 6 06:11:54 UTC 2019
Hi Vladimir,
Please find below updated patch link.
http://cr.openjdk.java.net/~kkharbas/ElementalRotate/webrev.02
Thanks,
Jatin
> -----Original Message-----
> From: Vladimir Ivanov [mailto:vladimir.x.ivanov at oracle.com]
> Sent: Saturday, March 2, 2019 3:18 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.
> FTR no need to rely on me to push patches. Some of your colleagues at Intel
> have Committer rights and can push it for you until you become Committer
> yourself.
>
> I went forward with the push, but spotted a test failure during a test run:
> FAILED: jdk/incubator/vector/VectorHash.java
>
> The following patch [1] fixes the failure, but there are other occurences in
> benchmarks [2]. Please, fix those as well.
>
> [1]
> diff --git a/test/jdk/jdk/incubator/vector/VectorHash.java
> b/test/jdk/jdk/incubator/vector/VectorHash.java
> --- a/test/jdk/jdk/incubator/vector/VectorHash.java
> +++ b/test/jdk/jdk/incubator/vector/VectorHash.java
> @@ -173,7 +173,7 @@
>
> h = h * top_h_coeff + x.mul(v_h_coeff).addAll();
>
> - b = b.shiftEL(intSpecies.length());
> + b = b.shiftER(intSpecies.length());
> }
> }
>
> [2]
> test/jdk/jdk/incubator/vector/benchmark/src//main/java/benchmark/jdk/in
> cubator/vector/AbstractVectorBenchmark.java:
> var v2 = hi.reshape(to).shiftER(vlen);
> test/jdk/jdk/incubator/vector/benchmark/src//main/java/benchmark/jdk/in
> cubator/vector/AbstractVectorBenchmark.java:
> var v3 = v2.shiftER(i * vlen); // [0 0 ... 0 | 1 1 ... 1 | 0 0 ... 0]
> test/jdk/jdk/incubator/vector/benchmark/src//main/java/benchmark/jdk/in
> cubator/vector/AbstractVectorBenchmark.java:
> var vb = ((IntVector)(va.shiftEL(k *
> B64.length()).reshape(B64).cast(species))).and(0xFF);
> test/jdk/jdk/incubator/vector/benchmark/src//main/java/benchmark/jdk/in
> cubator/vector/SumOfUnsignedBytes.java:
> var accHi =
> ((IntVector)(acc.shiftEL(mid).reshape(S128).cast(I256))).and(0xFFFF); // high
> half as ints
>
> ...
>
> >> 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.
>
> Just wanted to illustrate how confusing is Vector.toString() output
> w.r.t. shiftE*/rotateE*: the vector elements "move" right while the
> operation has "L" in its name :-)
>
> Best regards,
> Vladimir Ivanov
>
> >>> 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