On Fri, 2 May 2025 06:16:03 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 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
