RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements

[Quan Anh Mai]

On Sun, 17 Apr 2022 14:35:14 GMT, Jie Fu <jiefu at openjdk.org> wrote:

> Hi all,
> 
> According to the Vector API doc, the `LSHR` operator computes `a>>>(n&(ESIZE*8-1))`.
> However, current implementation is incorrect for negative bytes/shorts.
> 
> The background is that one of our customers try to vectorize `urshift` with `urshiftVector` like the following.
> 
>  13     public static void urshift(byte[] src, byte[] dst) {
>  14         for (int i = 0; i < src.length; i++) {
>  15             dst[i] = (byte)(src[i] >>> 3);
>  16         }
>  17     }
>  18 
>  19     public static void urshiftVector(byte[] src, byte[] dst) {
>  20         int i = 0;
>  21         for (; i < spec.loopBound(src.length); i +=spec.length()) {
>  22             var va = ByteVector.fromArray(spec, src, i);
>  23             var vb = va.lanewise(VectorOperators.LSHR, 3);
>  24             vb.intoArray(dst, i);
>  25         }
>  26 
>  27         for (; i < src.length; i++) {
>  28             dst[i] = (byte)(src[i] >>> 3);
>  29         }
>  30     }
> 
> 
> Unfortunately and to our surprise, code at line28 computes different results with code at line23.
> It took quite a long time to figure out this bug.
> 
> The root cause is that current implemenation of Vector API can't compute the  unsigned right shift results as what is done for scalar `>>>` for negative byte/short elements.
> Actually, current implementation will do `(a & 0xFF) >>> (n & 7)` [1] for all bytes, which is unable to compute the vectorized `>>>` for negative bytes.
> So this seems unreasonable and unfriendly to Java developers.
> It would be better to fix it.
> 
> The key idea to support unsigned right shift of negative bytes/shorts is just to replace the unsigned right shift operation with the signed right shift operation.
> This logic is:
> - For byte elements, unsigned right shift is equal to signed right shift if the shift_cnt <= 24.
> - For short elements, unsigned right shift is equal to signed right shift if the shift_cnt <= 16.
> - For Vector API, the shift_cnt will be masked to shift_cnt <= 7 for bytes and shift_cnt <= 15 for shorts.
> 
> I just learned this idea from https://github.com/openjdk/jdk/pull/7979 .
> And many thanks to @fg1417 .
> 
> 
> Thanks.
> Best regards,
> Jie
> 
> [1] https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/ByteVector.java#L935

Hi,

The `>>>` operator is not defined for subword types, what the code in line 28 does vs what it is supposed to do is different, which is more likely the bug here. An unsigned shift should operate on subword types the same as it does on word and double-word types, which is to zero extend the value before shifting it rightwards.

Another argument would be that an unsigned shift operates on the unsigned types, and the signed cast exposes this misunderstanding regarding the operation.

Thanks.

-------------

PR: https://git.openjdk.java.net/jdk/pull/8276