On Mon, 7 Jul 2025 06:19:15 GMT, Emanuel Peter <epe...@openjdk.org> wrote:
>> erifan has updated the pull request with a new target base due to a merge or >> a rebase. The incremental webrev excludes the unrelated changes brought in >> by the merge/rebase. The pull request contains 15 additional commits since >> the last revision: >> >> - Address more comments >> >> ATT. >> - Merge branch 'master' into JDK-8354242 >> - Support negating unsigned comparison for BoolTest::mask >> >> Added a static method `negate_mask(mask btm)` into BoolTest class to >> negate both signed and unsigned comparison. >> - Addressed some review comments >> - Merge branch 'master' into JDK-8354242 >> - Refactor the JTReg tests for compare.xor(maskAll) >> >> Also made a bit change to support pattern `VectorMask.fromLong()`. >> - Merge branch 'master' into JDK-8354242 >> - Refactor code >> >> Add a new function XorVNode::Ideal_XorV_VectorMaskCmp to do this >> optimization, making the code more modular. >> - Merge branch 'master' into JDK-8354242 >> - Update the jtreg test >> - ... and 5 more: https://git.openjdk.org/jdk/compare/8e600a2f...5ebdc572 > > src/hotspot/share/opto/vectornode.cpp line 2243: > >> 2241: !VectorNode::is_all_ones_vector(in2)) { >> 2242: return nullptr; >> 2243: } > > Suggestion: > > if (in1->Opcode() != Op_VectorMaskCmp || > in1->outcnt() != 1 || > !(in1->as_VectorMaskCmp())->predicate_can_be_negated() || > !VectorNode::is_all_ones_vector(in2)) { > return nullptr; > } > > Indentation for clarity. > > Currently, you exiting if one of these is the case: > 1. Not `MaskCmp` > 2. More than one use > 3. predicate cannot be negated AND the vector is all ones. Can you explain > this condition? Do you have tests for cases: > - predicate negatable and vector not all ones > - predircate not negatable and vector not all ones > - predicate negatable and vector all ones > - predicate not negatable and vectors all ones > > Why do you guard against `VectorNode::is_all_ones_vector(in2)` at all? > > The condition for 3. is easy to get wrong, so good testing is important here > :) The current testing status for the conditions you listed: > 1. Not MaskCmp. **No test for it, tested locally**, Because I think this condition is too straightforward. > 2. More than one use. **Tested**, see `VectorMaskCompareNotTest.java line 1118`. > predicate negatable and vector not all ones. **Tested**, see `VectorMaskCompareNotTest.java line 1126`. > predicate not negatable and vector not all ones. **No test for it**, because we have tests for `predicate not negatable` or `vector not all ones`. If either is `false`, return nullptr. > predicate negatable and vector all ones. **A lot of tests for it**. For example `VectorMaskCompareNotTest.java line 1014`. > predicate not negatable and vectors all ones. **Tested**, see `VectorMaskCompareNotTest.java line 1222`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2189495935
