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

[erifan]

On Tue, 9 Sep 2025 12:56:40 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> erifan has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Update the code comment
>
> src/hotspot/share/opto/vectornode.cpp line 2243:
> 
>> 2241:   if (in1->Opcode() != Op_VectorMaskCmp ||
>> 2242:       in1->outcnt() != 1 ||
>> 2243:       !(in1->as_VectorMaskCmp())->predicate_can_be_negated() ||
> 
> Suggestion:
> 
>       !in1->as_VectorMaskCmp()->predicate_can_be_negated() ||
> 
> Brackets are unnecessary, and rather make it harder to read.

Good catch, done.

> src/hotspot/share/opto/vectornode.cpp line 2277:
> 
>> 2275:     res = VectorNode::Ideal(phase, can_reshape);
>> 2276:   }
>> 2277:   return res;
> 
> What if someone comes and wants to add yet another optimization before `VectorNode::Ideal`? Your code layout would give us deeper and deeper nesting. I suggest flattening it like this:
> Suggestion:
> 
> 
>   Node* res = Ideal_XorV_VectorMaskCmp(phase, can_reshape);
>   if (res != nullptr) { return res; }
> 
>   return VectorNode::Ideal(phase, can_reshape);

Make sense, done.

> test/micro/org/openjdk/bench/jdk/incubator/vector/MaskCompareNotBenchmark.java line 351:
> 
>> 349:     public void testCompareULEMaskNotLong() {
>> 350:         testCompareMaskNotLong(VectorOperators.ULE);
>> 351:     }
> 
> You could consider making the operator a `@Param` next time.
> 
> There are multiple tricks to do that:
> - `test/micro/org/openjdk/bench/vm/compiler/VectorStoreToLoadForwarding.java` using `MethodHandles.constant`
> - Some inner class that has a static final, which is initialized from the non-final `@Param` value.
> - Probably even `StableValue` would work, but I have not yet experimented with it.
> 
> It would be nice if we could do the same with the primitive types, but that's probably not going to work as easily.
> 
> Really just an idea for next time.

Good point, I didn't know about these methods before. I will submit this change in my next commit, thank you.

> test/micro/org/openjdk/bench/jdk/incubator/vector/MaskCompareNotBenchmark.java line 366:
> 
>> 364:     public void testCompareNEMaskNotFloat() {
>> 365:         testCompareMaskNotFloat(VectorOperators.NE);
>> 366:     }
> 
> You could still add the other comparisons as well, so we can see the performance difference. Very optional, feel free to ignore this suggestion.

Sounds good, this will be added with the above change.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2335413222
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2335421260
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2335825557
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2335827904