On Tue, 9 Sep 2025 12:56:40 GMT, Emanuel Peter <[email protected]> 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
