RFR: 8338021: Support saturating vector operators in VectorAPI [v5]

[Sandhya Viswanathan]

On Mon, 2 Sep 2024 12:20:59 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> Hi All,
>> 
>> As per the discussion on panama-dev mailing list[1], patch adds the support 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:
> 
>   Review comments resolved

Thanks for considering the review comments. I have some minor follow ups.

src/hotspot/cpu/x86/assembler_x86.cpp line 8470:

> 8468: void Assembler::vpmaxud(XMMRegister dst, XMMRegister nds, XMMRegister src, int vector_len) {
> 8469:   assert(vector_len == AVX_128bit ? VM_Version::supports_avx() :
> 8470:         (vector_len == AVX_256bit ? VM_Version::supports_avx2() : VM_Version::supports_avx512bw()), "");

avx512bw check here seems wrong.

src/hotspot/cpu/x86/assembler_x86.cpp line 8479:

> 8477: void Assembler::vpmaxud(XMMRegister dst, XMMRegister nds, Address src, int vector_len) {
> 8478:   assert(vector_len == AVX_128bit ? VM_Version::supports_avx() :
> 8479:         (vector_len == AVX_256bit ? VM_Version::supports_avx2() : VM_Version::supports_avx512bw()), "");

avx512bw check here seems wrong.

src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 945:

> 943:  } else {
> 944:    vpblendvb(dst, src2, src1, xtmp1, vlen_enc);
> 945:  }

The comment needs to move inside if and else block as the code in these blocks is reverse of each other.

src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 6691:

> 6689:   // Res = INP1 - INP2 (non-commutative and non-associative)
> 6690:   // Res = Mask ? Zero : Res
> 6691:   evmasked_op(etype == T_INT ? Op_SubVI : Op_SubVL, etype, ktmp, dst, src1, src2, false, vlen_enc, false);

Do the comments need update here?
e.g. 6688 is setting mask bits to true for src2 <u src1 which is no_overflow
and 6690 is thus Res = mask ? (Inp1 - Inp2) : Zero

src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 6715:

> 6713:                                                         int vlen_enc) {
> 6714:   // Unsigned values ranges comprise of only +ve numbers, thus there exist only an upper bound saturation.
> 6715:   // overflow_mask = (SRC1 + SRC2) <u (SRC1 | SRC2)

Thanks, xtmp2 is no more used so could be removed.

src/java.base/share/classes/java/lang/Long.java line 1987:

> 1985:     public static long addSaturating(long a, long b) {
> 1986:         long res = a + b;
> 1987:         // HD 2-12 Overflow iff both arguments have the opposite sign of the result

HD -> Hacker's Delight

src/java.base/share/classes/java/lang/Long.java line 2008:

> 2006:     public static long subSaturating(long a, long b) {
> 2007:         long res = a - b;
> 2008:         // HD 2-12 Overflow iff the arguments have different signs and

HD -> Hacker's Delight

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

PR Review: https://git.openjdk.org/jdk/pull/20507#pullrequestreview-2277917757
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1742347879
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1742348218
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1742725069
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1742733746
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1742751114
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1742452009
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1742452802