On Mon, 21 Oct 2024 12:25:37 GMT, Jatin Bhateja <jbhat...@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
