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 Ok, I left a few more comments. I think this PR could definately be split. It would make it more reviewable for me. src/hotspot/share/opto/vectornode.hpp line 148: > 146: > 147: > //===========================Vector=ALU=Operations============================= > 148: class SaturatingVectorNode : public VectorNode { Semantics description of Saturation would be appreciated :) src/hotspot/share/opto/vectornode.hpp line 634: > 632: virtual int Opcode() const; > 633: }; > 634: This could also be a separate PR. Or are they somehow inseparable from the "saturation" changes? src/hotspot/share/prims/vectorSupport.hpp line 129: > 127: VECTOR_OP_SUSUB = 122, > 128: VECTOR_OP_UMIN = 123, > 129: VECTOR_OP_UMAX = 124, Please keep the alignment consistent. src/java.base/share/classes/java/lang/Integer.java line 1994: > 1992: * @return the greater of {@code a} and {@code b} > 1993: * @see java.util.function.BinaryOperator > 1994: * @since 1.8 Is this a copy error or did this already exist since `1.8`? src/java.base/share/classes/jdk/internal/vm/vector/VectorSupport.java line 395: > 393: > 394: /* > ============================================================================ > */ > 395: These comment lines seem redundant... test/jdk/jdk/incubator/vector/gen-template.sh line 317: > 315: function gen_saturating_binary_op { > 316: echo "Generating binary op $1 ($2)..." > 317: # gen_op_tmpl $binary_scalar "$@" Is this commented on purpose? ------------- Changes requested by epeter (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20507#pullrequestreview-2277361678 PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1742016482 PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1742019985 PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1742021810 PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1742024534 PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1742026062 PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1742028394
