On Mon, 7 Jul 2025 09:32:42 GMT, erifan <d...@openjdk.org> wrote:
>> src/hotspot/share/opto/vectornode.cpp line 2243:
>>
>>> 2241: !VectorNode::is_all_ones_vector(in2)) {
>>> 2242: return nullptr;
>>> 2243: }
>>
>> Suggestion:
>>
>> if (in1->Opcode() != Op_VectorMaskCmp ||
>> in1->outcnt() != 1 ||
>> !(in1->as_VectorMaskCmp())->predicate_can_be_negated() ||
>> !VectorNode::is_all_ones_vector(in2)) {
>> return nullptr;
>> }
>>
>> Indentation for clarity.
>>
>> Currently, you exiting if one of these is the case:
>> 1. Not `MaskCmp`
>> 2. More than one use
>> 3. predicate cannot be negated AND the vector is all ones. Can you explain
>> this condition? Do you have tests for cases:
>> - predicate negatable and vector not all ones
>> - predircate not negatable and vector not all ones
>> - predicate negatable and vector all ones
>> - predicate not negatable and vectors all ones
>>
>> Why do you guard against `VectorNode::is_all_ones_vector(in2)` at all?
>>
>> The condition for 3. is easy to get wrong, so good testing is important here
>> :)
>
> The current testing status for the conditions you listed:
>> 1. Not MaskCmp.
>
> **No test for it, tested locally**, Because I think this condition is too
> straightforward.
>
>> 2. More than one use.
>
> **Tested**, see `VectorMaskCompareNotTest.java line 1118`.
>
>> predicate negatable and vector not all ones.
>
> **Tested**, see `VectorMaskCompareNotTest.java line 1126`.
>
>> predicate not negatable and vector not all ones.
>
> **No test for it**, because we have tests for `predicate not negatable` or
> `vector not all ones`. If either is `false`, return nullptr.
>
>> predicate negatable and vector all ones.
>
> **A lot of tests for it**. For example `VectorMaskCompareNotTest.java line
> 1014`.
>
>> predicate not negatable and vectors all ones.
>
> **Tested**, see `VectorMaskCompareNotTest.java line 1222`.
> Indentation for clarity.
Done.
I think we have enough negative tests. Please take a look at this PR, thanks~
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2191550171
