On Wed, 28 May 2025 12:03:48 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 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
