RFR: 8338021: Support new unsigned and saturating vector operators in VectorAPI [v31]

[Emanuel Peter]

On Mon, 21 Oct 2024 12:25:37 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> Hi All,
>> 
>> As per the discussion on panama-dev mailing list[1], patch adds the support for following new vector operators.
>> 
>> 
>>      . SUADD   : Saturating unsigned addition.
>>      . SADD    : Saturating signed addition. 
>>      . SUSUB   : Saturating unsigned subtraction.
>>      . SSUB    : Saturating signed subtraction.
>>      . UMAX    : Unsigned max
>>      . UMIN    : Unsigned min.
>>      
>> 
>> New vector operators are applicable to only integral types since their values wraparound in over/underflowing scenarios after setting appropriate status flags. For floating point types, as per IEEE 754 specs there are multiple schemes to handler underflow, one of them is gradual underflow which transitions the value to subnormal range. Similarly, overflow implicitly saturates the floating-point value to an Infinite value.
>> 
>> As the name suggests, these are saturating operations, i.e. the result of the computation is strictly capped by lower and upper bounds of the result type and is not wrapped around in underflowing or overflowing scenarios.
>> 
>> Summary of changes:
>> - Java side implementation of new vector operators.
>> - Add new scalar saturating APIs for each of the above saturating vector operator in corresponding primitive box classes, fallback implementation of vector operators is based over it.
>> - C2 compiler IR and inline expander changes.
>> - Optimized x86 backend implementation for new vector operators and their predicated counterparts.
>> - Extends existing VectorAPI Jtreg test suite to cover new operations.
>> 
>> Kindly review and share your feedback.
>> 
>> Best Regards,
>> PS: Intrinsification and auto-vectorization of new core-lib API will be addressed separately in a follow-up patch.
>> 
>> [1] https://mail.openjdk.org/pipermail/panama-dev/2024-May/020408.html
>
> Jatin Bhateja has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Factor out IR tests and Transforms to follow-up PRs.

Ok, now it looks better.

I would still have preferred the `VectorMath` class with its test to be in a separate PR. Please do that next time.

I have a few more questions / comments, but we are really close now.
I'm especially worried about only testing a few constant values - we should try to go for better coverage - of all values if possible.

src/hotspot/cpu/x86/x86.ad line 10790:

> 10788:   predicate(is_subword_type(Matcher::vector_element_basic_type(n)) &&
> 10789:             n->is_SaturatingVector() && !n->as_SaturatingVector()->is_unsigned());
> 10790:   match(Set dst (SaturatingAddV (Binary dst (LoadVector src)) mask));

Do equivalent store operations exist we could also match for?

test/jdk/jdk/incubator/vector/VectorMathTest.java line 70:

> 68:     public static short[] INPUT_SS = {Short.MIN_VALUE,   (short)(Short.MIN_VALUE + TEN_S), ZERO_S, (short)(Short.MAX_VALUE - TEN_S), Short.MAX_VALUE};
> 69:     public static int[]   INPUT_SI = {Integer.MIN_VALUE, (Integer.MIN_VALUE + TEN_I),      ZERO_I, Integer.MAX_VALUE - TEN_I,        Integer.MAX_VALUE};
> 70:     public static long[]  INPUT_SL = {Long.MIN_VALUE,    Long.MIN_VALUE + TEN_L,           ZERO_L, Long.MAX_VALUE - TEN_L,           Long.MAX_VALUE};

Ok, now we have 4 or 5 hand-crafted examples. Is that sufficient? Some random values would be nice, then we know that at least eventually we have full coverage.

test/jdk/jdk/incubator/vector/templates/Kernel-SaturatingBinary-Masked-op.template line 8:

> 6: 
> 7:         for (int ic = 0; ic < INVOC_COUNT; ic++) {
> 8:             for (int i = 0; i < a.length; i += SPECIES.length()) {

I think this does not check if the generated vectors are too long. We had bugs in the past where we should have created say 2-element vectors, but the backend wrongly created 4-element vectors. This is especially an issue with vectors that do direct memory access.

With a simple "counting-up" test, you will probably not catch this. It could be good to have a "counting-down" example as well. What do you think?

test/jdk/jdk/incubator/vector/templates/Unit-header.template line 1244:

> 1242:                 return fill(s * BUFFER_REPS,
> 1243:                             i -> ($type$)($Boxtype$.MIN_VALUE + 100));
> 1244:             })

Not sure I see this right. But are we only providing these 4 constants as inputs, and all values in the input arrays will be identical? If that is true: we should have some random inputs, or at least dependent on `i`.

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

PR Review: https://git.openjdk.org/jdk/pull/20507#pullrequestreview-2391445047
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1814366518
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1814355931
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1814363960
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1814371488