On Wed, 11 Jun 2025 04:56:36 GMT, Emanuel Peter <epe...@openjdk.org> wrote:
>> erifan has updated the pull request incrementally with one additional commit >> since the last revision: >> >> 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. > > src/hotspot/share/opto/subnode.hpp line 333: > >> 331: mask negate( ) const { return mask(_test^4); } >> 332: // Return the negative mask for the given mask, for both signed and >> unsigned comparison. >> 333: static mask negate_mask(mask btm) { return mask(btm^4); } > > Suggestion: > > static mask negate_mask(mask btm) { return mask(btm ^ 4); } > > > https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md > >> Use spaces around operators, especially comparisons and assignments. >> (Relaxable for boolean expressions and high-precedence operators in classic >> math-style formulas.) Done, thanks! > src/hotspot/share/opto/vectornode.cpp line 2226: > >> 2224: >> 2225: const TypeVect* vector_mask_cast_vt = nullptr; >> 2226: // in1 should be single used, otherwise the optimization may be >> unprofitable. > > Suggestion: > > // in1 should only have a single use, otherwise the optimization may be > unprofitable. Done > src/hotspot/share/opto/vectornode.cpp line 2237: > >> 2235: !VectorNode::is_all_ones_vector(in2)) { >> 2236: return nullptr; >> 2237: } > > Similarly here: do you have tests for these conditions, that we do not > optimize if any of these fail? Added some negative tests for these conditions > src/hotspot/share/opto/vectornode.cpp line 2239: > >> 2237: } >> 2238: >> 2239: BoolTest::mask neg_cond = >> BoolTest::negate_mask(((VectorMaskCmpNode*) in1)->get_predicate()); > > Suggestion: > > BoolTest::mask neg_cond = > BoolTest::negate_mask((in1->as_VectorMaskCmp())->get_predicate()); > > Does that compile? It would be prefereable. Yes, done, thanks! > src/hotspot/share/opto/vectornode.cpp line 2243: > >> 2241: const TypeVect* vt = in1->as_Vector()->vect_type(); >> 2242: Node* res = new VectorMaskCmpNode(neg_cond, in1->in(1), in1->in(2), >> 2243: predicate_node, vt); > > Suggestion: > > Node* res = new VectorMaskCmpNode(neg_cond, in1->in(1), in1->in(2), > predicate_node, vt); > > Alignment Done > test/hotspot/jtreg/compiler/vectorapi/VectorMaskCompareNotTest.java line 244: > >> 242: testCompareMaskNotByte(VectorOperators.EQ, m -> m.not()); >> 243: testCompareMaskNotByte(VectorOperators.EQ, m -> >> m.xor(B_SPECIES.maskAll(true))); >> 244: } > > Could it happen that the verification is inlined in the test body? > > Currently, the verification is probably inlined, but the code there is not > vectorized. But what if one day the auto-vectorizer is smart enough and > vectorizes it, and creates vectors that we currently check `count ...= 0`? > > At least, you could ensure that the verification does not get inlined, with > `@DontInline`. > > What do you think? Make sense, done. > test/hotspot/jtreg/compiler/vectorapi/VectorMaskCompareNotTest.java line 692: > >> 690: TestFramework testFramework = new TestFramework(); >> 691: testFramework.addFlags("--add-modules=jdk.incubator.vector"); >> 692: testFramework.setDefaultWarmup(10000); > > The default is `2000` is that not enough? > > Increasing it means the test runs slower, here probably about 5x. Yes, not enough, changed to 5000. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2166343933 PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2166344643 PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2166346170 PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2166346620 PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2166346885 PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2166347867 PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2166352227