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