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
