[vectorIntrinsics] Issue to VectorAPI "selectFrom" for byte type with Arm SVE 2048-bits

[Paul Sandoz]

> On Oct 26, 2020, at 8:17 PM, Xiaohong Gong <Xiaohong.Gong at arm.com> wrote:
> 
> Hi Paul,
> 
> Thanks very much for your detailed explanation!
> 
>> 1) the selectFrom(Vector ) methods will never be able to index anything greater than the 127th lane element, if supported by the species. I think we would require the notion of a Vector whose lane elements are unsigned bytes, which I think requires Valhalla and additional numeric types.
> This limitation would obviously become even more apparent if say a future SVE specification supported vector shapes of 4096 bits.
> 
> Yes, I agree with you. And it also makes sense to me that only the unsigned bytes can be the valid elements for this API, even the larger vector shapes might be supported like SVE 2048-bits.
> So regarding to the test failures, I think we'd better to limit the lane elements to the valid values to make the tests pass.  I will raise a patch for it if it's ok to you.
> 

Yes, clamp to Byte.MAX_VALUE for a maximum valid index on relevant platforms.


>> Because of 3) we cannot directly create a VectorShuffle for a 2048:byte species that supports indexes > 127. That seems fixable by modifying the implementation of ByteMaxShuffle to hold short[] instead of byte[], or byte[] twice the length etc. That may require some care with regards to the intrinsic. This seems possible.
> 
> Yes, exactly. Currently we have created a patch to fix this issue. And the main idea is the same with https://mail.openjdk.java.net/pipermail/panama-dev/2020-August/010384.html.
> 

Ok, I’ll wait for a PR, but I wonder if its possible to focus the code more on the ByteMax implementations.

Paul;.

> Thanks,
> Xiaohong Gong
> 
> 
> -----Original Message-----
> From: Paul Sandoz <paul.sandoz at oracle.com> 
> Sent: Tuesday, October 27, 2020 7:47 AM
> To: Xiaohong Gong <Xiaohong.Gong at arm.com>
> Cc: panama-dev at openjdk.java.net; nd <nd at arm.com>
> Subject: Re: [vectorIntrinsics] Issue to VectorAPI "selectFrom" for byte type with Arm SVE 2048-bits
> 
> Hi,
> 
> I think there are a few things going on here:
> 
> 1) Vector<Byte> holds signed values (as do the other integral lane element types) so we can always produce negative out of bounds values when such a Vector’s lane elements are used to index into another Vector of the same species.
> 
> 2) The VectorShuffle specification supports negative “exceptional” values representing invalid lane indexes, or access to a second vector.
> 
> 3) The VectorShuffle implementation currently uses byte[] as the underlying storage.
> 
> 
> Because of 1) the selectFrom(Vector ) methods will never be able to index anything greater than the 127th lane element, if supported by the species. I think we would require the notion of a Vector whose lane elements are unsigned bytes, which I think requires Valhalla and additional numeric types.
> This limitation would obviously become even more apparent if say a future SVE specification supported vector shapes of 4096 bits.
> 
> Given that this.selectFrom(Vector that) is equivalent to that.rearrange(this.toShuffle()) we might be able to remove selectFrom(Vector ), but we likely need to teach C2 to recognize the pattern of the latter and elide the shuffle conversion if there is a more direct instruction.
> 
> Because of 3) we cannot directly create a VectorShuffle for a 2048:byte species that supports indexes > 127. That seems fixable by modifying the implementation of ByteMaxShuffle to hold short[] instead of byte[], or byte[] twice the length etc. That may require some care with regards to the intrinsic. This seems possible.
> 
> Paul.
> 
> 
>> On Oct 22, 2020, at 11:59 PM, Xiaohong Gong <Xiaohong.Gong at arm.com> wrote:
>> 
>> Hi,
>> 
>> Currently the vector API test "SelectFromByteMaxVectorTests" throws with "IndexOutOfBoundsException"
>> when run with Arm SVE 2048-bits.  The tests are used to test the two APIs:
>> 
>> ```
>> // Using index values stored in the lanes of this vector, assemble values stored in second vector v.
>> public abstract Vector<E> selectFrom(Vector<E> v); // Using index 
>> values stored in the lanes of this vector, assemble values stored in 
>> second vector, under the  // control of a mask.
>> public abstract Vector<E> selectFrom(Vector<E> v, VectorMask<E> m); 
>> ```
>> 
>> Just as the notes described, the vector itself stores the index values 
>> in the lanes and returns a vector whose elements are from the "vector v" under the control of the index values. It works just like the "VectorShuffle".
>> So for the vector itself, the element values should be in the range of 
>> 0 ~ (vector_length - 1).  And since the two APIs are defined in all 
>> vector types (byte, short, int, long, float, double),  the element type is the relative primitive type.  This can work fine for all types if the vector length is lower than the max value of the type.
>> Otherwise, the index value might not be the right one due to type cast.
>> 
>> So here is the root cause for the test exception:  for SVE 2048-bits, 
>> the max vector length for byte is 256
>> (2048 / 8). The valid index values are in the range of 0 ~ 255. 
>> However, since the values are stored to a byte vector with the range of -128 ~ 127, the final values might be negative.  So the exception happens like:
>> 
>> ```
>> test 
>> ByteMaxVectorTests.SelectFromByteMaxVectorTestsMaskedSmokeTest(byte[co
>> rnerCaseValue(i)], shuffle[random], mask[i % 2]): failure
>> java.lang.IndexOutOfBoundsException: required an index in [0..255] but found -116
>>   at jdk.incubator.vector/jdk.incubator.vector.AbstractShuffle.checkIndexFailed(AbstractShuffle.java:288)
>>   at jdk.incubator.vector/jdk.incubator.vector.AbstractShuffle.checkIndexes(AbstractShuffle.java:187)
>>   at jdk.incubator.vector/jdk.incubator.vector.ByteVector.rearrangeTemplate(ByteVector.java:2142)
>>   at jdk.incubator.vector/jdk.incubator.vector.ByteMaxVector.rearrange(ByteMaxVector.java:478)
>>   at jdk.incubator.vector/jdk.incubator.vector.ByteMaxVector.rearrange(ByteMaxVector.java:43)
>>   at jdk.incubator.vector/jdk.incubator.vector.ByteVector.selectFromTemplate(ByteVector.java:2208)
>>   at jdk.incubator.vector/jdk.incubator.vector.ByteMaxVector.selectFrom(ByteMaxVector.java:505)
>>   at jdk.incubator.vector/jdk.incubator.vector.ByteMaxVector.selectFrom(ByteMaxVector.java:43)
>>   at 
>> ByteMaxVectorTests.SelectFromByteMaxVectorTestsMaskedSmokeTest(ByteMax
>> VectorTests.java:5058)
>> ```
>> 
>> One way to fix this issue is to use a more appropriate type (like int) 
>> to store the index values while not the relevant vector type.  And 
>> since the values in the lanes are used as the vector index,  is it possible to only define the APIs for int type?  Many thanks if I can get any comments or better ideas about this issue!
>> 
>> Thanks,
>> Xiaohong Gong
>