On Mon, 21 Oct 2024 09:12:25 GMT, Emanuel Peter <epe...@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. 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 ------------- PR Comment: https://git.openjdk.org/jdk/pull/20507#issuecomment-2426527459
