On Mon, 28 Apr 2025 14:06:49 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 four additional commits since 
>> the last revision:
>> 
>>  - 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     
>> 10245063.58     9774.75138      1.29
>>    testCompareGTMaskNotInt           ops/s   1673393...
>
> test/hotspot/jtreg/compiler/vectorapi/VectorMaskCompareNotTest.java line 237:
> 
>> 235:     public static void testCompareEQMaskNotByte() {
>> 236:         testCompareMaskNotByte(VectorOperators.EQ);
>> 237:     }
> 
> Another comment: you now only have "negative" tests, where you check for 
> count `=0`. It would be good if you also had a positive rule here, one where 
> you do see an XOR in a similar case, where your optimization does apply.
> 
> This would basically be a "control" that checks that your are testing the 
> right thing.

Also: this test should vectorize on some plarforms, right? A compare, correct? 
Would it not be good to actively check that with an IR rule?

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

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

Reply via email to