On Fri, 18 Oct 2024 14:56:29 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: > > Replacing flag based checks with CPU feature checks in IR validation test. Wow this is really a very moving target - quite frustrating to review - it takes up way too much of the reviewers bandwidth. You really need to split up your PRs as much as possible so that review is easier and faster. I think these optimizations should be done in a separate PR. I see no reason why they need to be mixed in. https://github.com/openjdk/jdk/commit/c56508899b000b8b1eb6755c901798a2a3685ef5 The `UMinVNode::Ideal` etc changes with IR rules. I also cannot easily review just such a diff, it does not let me make comments - so I still have to go find the relevant code in the whole PR. Some comments on this section: Node* UMinVNode::Ideal(PhaseGVN* phase, bool can_reshape) { bool match1 = in(1)->Opcode() == Op_UMinV || in(1)->Opcode() == Op_UMaxV; bool match2 = in(2)->Opcode() == Op_UMinV || in(2)->Opcode() == Op_UMaxV; // UMin (UMin(a, b), UMax(a, b)) => UMin(a, b) // UMin (UMin(a, b), UMax(b, a)) => UMin(a, b) if (match1 && match2) { if ((in(1)->in(1) == in(2)->in(1) && in(1)->in(2) == in(2)->in(2)) || (in(1)->in(2) == in(2)->in(1) && in(1)->in(1) == in(2)->in(2))) { return new UMinVNode(in(1)->in(1), in(1)->in(2), vect_type()); } } return nullptr; } What about the reverse case `min(max, min)`? And are we sure we do not need to verify any types in all of these cases? Maybe not - but I'd rather be super sure - not that things get misinterpreted and then folded the wrong way. I mean if I now approve only that diff, then I still need to approve the whole PR, which means I need to spend a lot of time on everything again. Otherwise, in theory people could smuggle anything in. ------------- PR Comment: https://git.openjdk.org/jdk/pull/20507#issuecomment-2426080652
