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

[erifan]

On Fri, 2 May 2025 06:16:03 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 six additional commits since the last revision:
>> 
>>  - 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	4094.243652	10251646.9	9486.699831	1.29
>>    testCompareGEMaskNotInt		ops/s	1683738.958	4233.813092	2352855.205	1251.952546	1.39
>>    testCompareGEMaskNotLong	ops/s	854496.1561	8594.598885	1177811.493	521.1229	1.37
>>    testCompareGEMaskNotShort	ops/s	3341860.309	1578.975338	4714008.434	1681.10365	1.41
>>    testCompareGTMaskNotByte	ops/s	7910823.674	2993.367032	1...
>
> src/hotspot/share/opto/vectornode.cpp line 2224:
> 
>> 2222:   //    => (VectorMaskCmp src1 src2 ncond)
>> 2223:   // cond can be eq, ne, le, ge, lt, gt, ule, uge, ult and ugt, ncond is the
>> 2224:   // negative comparison of cond.
> 
> Suggestion:
> 
>   // cond can be eq, ne, le, ge, lt, gt, ule, uge, ult and ugt.
>   // ncond is the negative comparison of cond.
> 
> I was getting lost in all the commas.

Done.

> src/hotspot/share/opto/vectornode.cpp line 2248:
> 
>> 2246:       !((VectorMaskCmpNode*) in1)->predicate_can_be_inverted() ||
>> 2247:       !VectorNode::is_all_ones_vector(in2)) {
>> 2248:     return VectorNode::Ideal(phase, can_reshape);
> 
> Hmm, so this is really the "else" case, if your optimization does not succeed, right?
> 
> Wrapping `VectorNode::Ideal` somewhere in the middle is going to make future optimizations here much harder.
> How would they check their conditions next to yours? That would be quite a mess.
> 
> I suggest you do this:
> - `XorVNode::Ideal` does
>   - checks `in1 == in2` case
>   - calls a method called `XorVNode::Ideal_XorV_VectorMaskCmp`. Check if it succeeded, i.e. returns `nullptr`.
>   - Finally, i.e. none of the optimizations above worked: call `VectorNode::Ideal`
> 
> Then you pack all your new logic here into `XorVNode::Ideal_XorV_VectorMaskCmp`. You can also find a better name, it is just what I came up with just now.
> 
> This gives us a much more **modular** design, and it is easier to add another new optimization to `XorVNode::Ideal`. It is easy to change the precedence of the optimizations by just changing the order, etc.

Done, thanks!

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

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