On Tue, 17 Sep 2024 07:14:57 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: > > Missed code fragment from last review comment resolution. Do we have tests for the publically exposed methods in this new file? Or are they only implicitly tested through the VectorAPI, and its tests? `src/jdk.incubator.vector/share/classes/jdk/incubator/vector/VectorMath.java` Why is this even called `VectorMath`? Because those ops are not at all restricted to vectorization, right? src/hotspot/cpu/x86/x86.ad line 6578: > 6576: %} > 6577: ins_pipe( pipe_slow ); > 6578: %} Above, you name both the `format` and method name with `_reg` and `_mem`, but here you do not do it for the method name. Would be nice to keep it consistent. src/hotspot/cpu/x86/x86.ad line 10793: > 10791: match(Set dst (SaturatingAddV (Binary dst (LoadVector src)) mask)); > 10792: match(Set dst (SaturatingSubV (Binary dst (LoadVector src)) mask)); > 10793: format %{ "vector_saturating_unsigned_masked $dst, $mask, $src" %} Suggestion: format %{ "vector_saturating_unsigned_subword_masked $dst, $mask, $src" %} src/hotspot/share/opto/vectornode.hpp line 81: > 79: static VectorNode* shift_count(int opc, Node* cnt, uint vlen, BasicType > bt); > 80: static VectorNode* make(int opc, Node* n1, Node* n2, uint vlen, > BasicType bt, bool is_var_shift = false); > 81: static VectorNode* make(int vopc, Node* n1, Node* n2, const TypeVect* > vt, bool is_mask = false, bool is_var_shift = false, bool is_unsigned = > false); Feels like this just slowly grows and grows... eventually we will have too many arguments. Not sure what is a better alternative though... src/hotspot/share/opto/vectornode.hpp line 386: > 384: class SaturatingSubVNode : public SaturatingVectorNode { > 385: public: > 386: SaturatingSubVNode(Node* in1, Node* in2, const TypeVect* vt, bool > is_unsigned) : SaturatingVectorNode(in1,in2,vt,is_unsigned) {} Suggestion: SaturatingSubVNode(Node* in1, Node* in2, const TypeVect* vt, bool is_unsigned) : SaturatingVectorNode(in1, in2, vt, is_unsigned) {} spaces required by style guide src/hotspot/share/opto/vectornode.hpp line 598: > 596: class UMinVNode : public VectorNode { > 597: public: > 598: UMinVNode(Node* in1, Node* in2, const TypeVect* vt) : > VectorNode(in1,in2,vt) { Suggestion: UMinVNode(Node* in1, Node* in2, const TypeVect* vt) : VectorNode(in1, in2, vt) { spaces required by style guide src/hotspot/share/opto/vectornode.hpp line 614: > 612: class UMaxVNode : public VectorNode { > 613: public: > 614: UMaxVNode(Node* in1, Node* in2, const TypeVect* vt) : > VectorNode(in1,in2,vt) { Suggestion: UMaxVNode(Node* in1, Node* in2, const TypeVect* vt) : VectorNode(in1, in2, vt) { spaces required by style guide ------------- Changes requested by epeter (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20507#pullrequestreview-2312554183 PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1764975321 PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1764982201 PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1764985438 PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1764987143 PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1764987547 PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1764988807
