RFR: 8354242: VectorAPI: combine vector not operation with compare [v8]

[erifan]

On Wed, 11 Jun 2025 04:56:36 GMT, Emanuel Peter <epeter at 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