On Tue, 22 Oct 2024 15:56:18 GMT, Paul Sandoz <psan...@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
