On Mon, 2 Sep 2024 12:20:59 GMT, Jatin Bhateja <jbhat...@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
