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

[erifan]

On Wed, 28 May 2025 12:03:48 GMT, Emanuel Peter <epeter at 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 10 additional commits since the last revision:
>> 
>>  - 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
>>  - Merge branch 'master' into JDK-8354242
>>  - Addressed some review comments
>>    
>>    1. Call VectorNode::Ideal() only once in XorVNode::Ideal.
>>    2. Improve code comments.
>>  - Merge branch 'master' into JDK-8354242
>>  - Merge branch 'master' into JDK-8354242
>>  - 8354242: VectorAPI: combine vector not operation with compare
>>    
>>    This patch optimizes the following patterns:
>>    For integer types:
>>    ```
>>    (XorV (VectorMaskCmp src1 src2 cond) (Replicate -1))
>>        => (VectorMaskCmp src1 src2 ncond)
>>    (XorVMask (VectorMaskCmp src1 src2 cond) (MaskAll m1))
>>        => (VectorMaskCmp src1 src2 ncond)
>>    ```
>>    cond can be eq, ne, le, ge, lt, gt, ule, uge, ult and ugt, ncond is the
>>    negative comparison of cond.
>>    
>>    For float and double types:
>>    ```
>>    (XorV (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (Replicate -1))
>>        => (VectorMaskCast (VectorMaskCmp src1 src2 ncond))
>>    (XorVMask (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (MaskAll m1))
>>        => (VectorMaskCast (VectorMaskCmp src1 src2 ncond))
>>    ```
>>    cond can be eq or ne.
>>    
>>    Benchmarks on Nvidia Grace machine with 128-bit SVE2:
>>    With option `-XX:UseSVE=2`:
>>    ```
>>    Benchmark			Unit	Before		Score Error	After		Score Error	Uplift
>>    testCompareEQMaskNotByte	ops/s	7912127.225	2677.289518	10266136.26	8955.008548	1.29
>>    testCompareEQMaskNotDouble	ops/s	884737.6799	446.963779	1179760.772	448.031844	1.33
>>    testCompareEQMaskNotFloat	ops/s	1765045.787	682.332214	2359520.803	896.305743	1.33
>>    testCompareEQMaskNotInt		ops/s	1787221.411	977.743935	2353952.519	960.069976	1.31
>>    testCompareEQMaskNotLong	ops/s	895297.1974	673.44808	1178449.02	323.804205	1.31
>>    testCompareEQMaskNotShort	ops/s	3339987.002	3415.2226	4712761.965	2110.862053	1.41
>>    testCompareGEMaskNotByte	ops/s	7907615.16	4...
>
> src/hotspot/share/opto/vectornode.cpp line 2213:
> 
>> 2211:   Node* in1 = in(1);
>> 2212:   Node* in2 = in(2);
>> 2213:   // Transformations for predicated IRs are not supported for now.
> 
> Suggestion:
> 
>   // Transformations for predicated vectors are not supported for now.

Done.

> src/hotspot/share/opto/vectornode.cpp line 2215:
> 
>> 2213:   // Transformations for predicated IRs are not supported for now.
>> 2214:   if (is_predicated_vector() || in1->is_predicated_vector() ||
>> 2215:       in2->is_predicated_vector()) {
> 
> I would either put all on the same line, or all on separate lines.

Done.

> src/hotspot/share/opto/vectornode.cpp line 2219:
> 
>> 2217:   }
>> 2218: 
>> 2219:   // XorV/XorVMask is commutative, swap VectorMaskCmp/Op_VectorMaskCast to in1.
> 
> Suggestion:
> 
>   // XorV/XorVMask is commutative, swap VectorMaskCmp/VectorMaskCast to in1.
> 
> Would look a little cleaner, and you did also not write `Op_VectorMaskCmp` either ;)

Done, thanks!

> src/hotspot/share/opto/vectornode.cpp line 2225:
> 
>> 2223:   }
>> 2224: 
>> 2225:   const TypeVect* vmcast_vt = nullptr;
> 
> Suggestion:
> 
>   const TypeVect* vector_mask_cast_vt = nullptr;
> 
> I think it would not hurt to write it out. Otherwise, the reader always has to reconstruct that in their head.

Done.

> src/hotspot/share/opto/vectornode.cpp line 2230:
> 
>> 2228:     vmcast_vt = in1->as_Vector()->vect_type();
>> 2229:     in1 = in1->in(1);
>> 2230:   }
> 
> Add a comment why you check `in1->outcnt() == 1`.

Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2128341063
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2128340484
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2128341959
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2128342468
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2128342908