RFR: 8338021: Support new unsigned and saturating vector operators in VectorAPI [v30]

[Jatin Bhateja]

On Mon, 21 Oct 2024 09:12:25 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> 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;
> }
> 
> 
> 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.

Hi @eme64 , Let me know if there are other comments, else looking forward to you approval :-)

-------------

PR Comment: https://git.openjdk.org/jdk/pull/20507#issuecomment-2436663568