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

[Jatin Bhateja]

On Tue, 22 Oct 2024 15:56:18 GMT, Paul Sandoz <psandoz at openjdk.org> wrote:

>> Hey @eme64 ,
>> 
>>> 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 understand reviewer's pain, which is why I mentioned about last two changes specifically. Vector API related PRs generally looks bulky due to script generated sources and tests. Barring that it may not demand much of your time.
>> 
>> But, to keep you motivated :-) and following @PaulSandoz and yours suggestions, I have moved out IR validations and Min / Max transforms to following follow up PRs.
>>    
>>    - https://bugs.openjdk.org/browse/JDK-8342676 (https://github.com/openjdk/jdk/pull/21604)
>>    - https://bugs.openjdk.org/browse/JDK-8342677 (https://github.com/openjdk/jdk/pull/21603)
>> 
>> Can you kindly run this though your test infrastructure and approve if it goes fine ?
>> 
>> Best Regards,
>> Jatin
>
>> Can you kindly run this though your test infrastructure and approve if it goes fine ?
>>
> 
> Internal tier 1 to 3 testing passed (i needed to merge with master at 7133d1b983d, due to some updates to unrelated test configuration files the test infrastructure expects).

> Ok, I'll approve it now, since @PaulSandoz is ok with the test coverage. I did not review all the x86 backend instructions, I don't have the time or understanding for that - I rely on testing for correctness here.
> 
> Thanks for the work @jatin-bhateja 😊 The VectorAPI is a lot of work, and we are grateful for your contributions!
> 
> I am still worried about general test coverage. One can always do it later - but will that actually be done? I suppose this is a trade-off between spending time on quality vs quantity of features. Bugs in the VectorAPI will probably mostly show in miscompilations. Those are hard to catch without solid testing and rigorous result verification. The VectorAPI is also not widely used yet, so miscompilations will only slowly be discovered. Miscompilations in Auto-Vectorization are quicker discovered because there is more code out there that is sensitive to it.
> 
> Maybe I'm coming across as annoying with all my RFE-splitting suggestions. But I do think that it is the job of the PR-author to make the code as reviewable and high-quality before it is sent for review. The author knows the code best, and should not burden the reviewers unnecessarily with combined features. A PR that is 2x as long is not just 2x as much work to review. It takes the reviewer more time - maybe 4x because it is harder to understand what belongs together, if everything is sufficiently tested. Also, the number of review-rounds increases. Every time the reviewer is then required to read all code again. This is really a waste of time and very frustrating. But I think you understand that now, and I am looking forward to nicely portioned PRs in the future ;)

We are in same boat @eme64 , I understand reviewers pain, and no debate on coverage and validations 👍 

@sviswa7 , I guess we need a cursory re-approval from you for integration.

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

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