On Mon, 7 Jul 2025 06:19:15 GMT, Emanuel Peter <epe...@openjdk.org> wrote:

>> erifan has updated the pull request with a new target base due to a merge or 
>> a rebase. The incremental webrev excludes the unrelated changes brought in 
>> by the merge/rebase. The pull request contains 15 additional commits since 
>> the last revision:
>> 
>>  - Address more comments
>>    
>>    ATT.
>>  - Merge branch 'master' into JDK-8354242
>>  - 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.
>>  - Addressed some review comments
>>  - Merge branch 'master' into JDK-8354242
>>  - Refactor the JTReg tests for compare.xor(maskAll)
>>    
>>    Also made a bit change to support pattern `VectorMask.fromLong()`.
>>  - Merge branch 'master' into JDK-8354242
>>  - Refactor code
>>    
>>    Add a new function XorVNode::Ideal_XorV_VectorMaskCmp to do this
>>    optimization, making the code more modular.
>>  - Merge branch 'master' into JDK-8354242
>>  - Update the jtreg test
>>  - ... and 5 more: https://git.openjdk.org/jdk/compare/8e600a2f...5ebdc572
>
> 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`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2189495935

Reply via email to