FW: [vector api] Vector elements shift confusion
Viswanathan, Sandhya
sandhya.viswanathan at intel.com
Mon Mar 11 22:43:15 UTC 2019
Hi Jatin,
Please find my review comments below:
1) Vector.java, rotateEL:
Line 552:
552 * {@code (length() + i) % length()}.
Should be:
552 * {@code (I + N) % length()}
2) Vector.java, rotateER:
Line 568:
568 * {@code (length() - N + i) % length()}.
Should be:
568 * {@code (length() - (N % length()) + i ) % length()}.
3) Vector.java, shiftEL:
580 * This is a cross-lane operation that permutes the lane elements of this
581 * vector and behaves as if rotating left the lane elements by {@code N},
582 * and then the zero value is placed into the result vector at lane indexes
583 * less than {@code N}.
Should be:
580 * This is a cross-lane operation that permutes the lane elements of this
581 * vector and behaves as if rotating left the lane elements by {@code N%length()},
582 * and then the zero value is placed into the result vector at lane indexes
583 * less than {@code N%length()}.
4) Vector.java, shiftER:
596 * This is a cross-lane operation that permutes the lane elements of this
597 * vector and behaves as if rotating right the lane elements by {@code N},
598 * and then the zero value is placed into the result vector at lane indexes
599 * less than {@code length() - N}.
Should be:
596 * This is a cross-lane operation that permutes the lane elements of this
597 * vector and behaves as if rotating right the lane elements by {@code N % length()},
598 * and then the zero value is placed into the result vector at lane indexes
599 * greater or equal to {@code length() - N % length()}.
5) X-VectorBits.java.template, shiftEL():
j = j % length(); should be added before the loop at line 1627
1627 for (int i = 0; i < length() - j; i++) {
1628 res[i + j] = vec[i];
1629 }
6) X-VectorBits.java.template, shiftER():
j = j%length(); should be added before the loop at line 1637
1637 for (int i = 0; i < length() - j; i++){
1638 res[i] = vec[i + j];
1639 }
7) The case when j is < 0 need to be handled for shiftEL, shiftER, rotateEL, rotateER and IllegalArgumentException thrown.
Best Regards,
Sandhya
-----Original Message-----
From: Bhateja, Jatin
Sent: Tuesday, March 05, 2019 10:12 PM
To: Vladimir Ivanov <vladimir.x.ivanov at oracle.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
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/i
> n
> cubator/vector/AbstractVectorBenchmark.java:
> var v2 = hi.reshape(to).shiftER(vlen);
> test/jdk/jdk/incubator/vector/benchmark/src//main/java/benchmark/jdk/i
> n
> 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/i
> n
> 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/i
> n
> 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