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

Thu Apr 24 09:51:45 UTC 2025

On Wed, 23 Apr 2025 12:09:51 GMT, Jatin Bhateja <jbhateja 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 two additional commits since the last revision:
>> 
>>  - 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.928	3153.099431	2353654.521	1190.848583	1.4
>>    testCompareGTMaskNotLong	ops/s	849405.9159	2432.858159	1177952.041	359.96413	1.38
>>    testCompareGTMaskNotShort	ops/s	3339509.141	...
>
> src/hotspot/share/opto/vectornode.cpp line 2234:
> 
>> 2232:   // XorV/XorVMask is commutative, swap VectorMaskCmp/Op_VectorMaskCast to in1.
>> 2233:   if (in1->Opcode() != Op_VectorMaskCmp && in1->Opcode() != Op_VectorMaskCast) {
>> 2234:     swap(in1, in2);
> 
> Swapping inputs like this without refreshing GVN bookkeeping is not safe. I guess you wanted to use Node::swap_edges.

The edges are not swapped, but two variables in1 and in2

> src/hotspot/share/opto/vectornode.cpp line 2243:
> 
>> 2241:     in1 = in1->in(1);
>> 2242:   }
>> 2243:   if (in1->Opcode() != Op_VectorMaskCmp || in1->outcnt() > 1 ||
> 
> Checks on outcnt on line 2243 and 2238 can be removed. Idealization looks for a specific graph palette and replaces it with a new node whose inputs are the same as the inputs of the palette. GVN will do the retention job if any intermediate node has users beyond the pattern being replaced.

Thanks for telling me this information. Another more important reason to check outcnt here is to prevent this optimization when the uses of VectorMaskCmp is greater than 1, because this optimization may not be worthwhile. For example:

  public static void testVectorMaskCmp() {
    IntVector bv = IntVector.fromArray(I_SPECIES, ib, 0);
    IntVector av = IntVector.fromArray(I_SPECIES, ia, 0);
    VectorMask<Integer> m1 = av.compare(VectorOperators.NE, bv);  // two uses
    VectorMask<Integer> m2 =m1.not();
    m1.intoArray(m, 0);
    av.lanewise(VectorOperators.ABS, m2).intoArray(ia, 0);
  }

If we do not check outcnt and still do this optimization, two VectorMaskCmp nodes will be generated, and finally two VectorMaskCmp instructions will be generated. This is unreasonable because VectorMaskCmp has much higher latency than xor instruction on aarch64.

> src/hotspot/share/opto/vectornode.cpp line 2265:
> 
>> 2263:     vmcmp = new VectorMaskCastNode(phase->transform(vmcmp), vmcast_vt);
>> 2264:   }
>> 2265:   return vmcmp;
> 
> It would be preferable if you could kindly re-factor the code such that we only call VectorNode::Ideal once at return to comply with aesthetics of other idealization routines.

Ok, I'll change it in the next commit.

> test/hotspot/jtreg/compiler/vectorapi/VectorMaskCompareNotTest.java line 38:
> 
>> 36:  * @summary test combining vector not operation with compare
>> 37:  * @modules jdk.incubator.vector
>> 38:  * @requires ((os.arch!="x86" & os.arch!="i386" & os.arch!="amd64" & os.arch!="x86_64") | vm.cpu.features ~= ".*avx.*")
> 
> You can remove this platform limitation and forward the constraints to @IR rules using applyIfCPUFeatureOr

Since this is a platform independent optimization, I tend to use this `@requires` because it's simpler.  If we use `applyIfCPUFeatureOr`, we need to add the same restriction before each test. In addition, if a new architecture supports the vector node, this test may not cover it. What do you think?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2057901771
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2057976512
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2057982231
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2057993569