[vector api] Vector elements shift confusion
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Thu Feb 28 05:21:15 UTC 2019
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