RFR: 8342676: Unsigned Vector Min / Max transforms [v2]

[Emanuel Peter]

On Tue, 7 Jan 2025 08:58:12 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> Adding following IR transforms for unsigned vector Min / Max nodes.
>> 
>> => UMinV (UMinV(a, b), UMaxV(a, b)) => UMinV(a, b)
>> => UMinV (UMinV(a, b), UMaxV(b, a)) => UMinV(a, b)
>> => UMaxV (UMinV(a, b), UMaxV(a, b)) => UMaxV(a, b)
>> => UMaxV (UMinV(a, b), UMaxV(b, a)) => UMaxV(a, b)
>> => UMaxV (a, a) => a
>> => UMinV (a, a) => a
>> 
>> New IR validation test accompanies the patch.
>> 
>> This is a follow-up PR for https://github.com/openjdk/jdk/pull/20507
>> 
>> Best Regards,
>> Jatin
>
> Jatin Bhateja 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 five additional commits since the last revision:
> 
>  - Updating copyright year of modified files
>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8342676
>  - Update IR transforms and tests
>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8342676
>  - 8342676: Unsigned Vector Min / Max transforms

Generally looks reasonable. Smaller changes like this are much easier to review, thank you for splitting it out 😊 

I started some testing, so feel free to ping me again for that.

src/hotspot/share/opto/vectornode.cpp line 2140:

> 2138: }
> 2139: 
> 2140: static Node* unsigned_min_max_xform(Node* n) {

Suggestion:

static Node* UMinMaxV_Ideal(Node* n) {

I think this is how we generally name functions when we share them between nodes of different types.

src/hotspot/share/opto/vectornode.cpp line 2162:

> 2160:   // UMax (UMin(a, b), UMax(a, b))  => UMax(a, b)
> 2161:   // UMax (UMax(a, b), UMin(b, a))  => UMax(a, b)
> 2162:   if (umin && umax) {

That looks like an implicit null check. Not allowed according to style guide:
`Do not use ints or pointers as (implicit) booleans with &&, ||, if, while. Instead, compare explicitly, i.e. if (x != 0) or if (ptr != nullptr), etc.`
https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md

src/hotspot/share/opto/vectornode.cpp line 2164:

> 2162:   if (umin && umax) {
> 2163:     if ((umin->in(1) == umax->in(1) && umin->in(2) == umax->in(2)) ||
> 2164:          (umin->in(2) == umax->in(1) && umin->in(1) == umax->in(2))) {

Suggestion:

    if ((umin->in(1) == umax->in(1) && umin->in(2) == umax->in(2)) ||
        (umin->in(2) == umax->in(1) && umin->in(1) == umax->in(2))) {

Alignment was off

src/hotspot/share/opto/vectornode.hpp line 622:

> 620:   virtual uint hash() const {
> 621:     return (uintptr_t)in(1) + (uintptr_t)in(2) + Opcode();
> 622:   }

Can you explain why you do this instead of `Node::hash`?
I think you do it so that you get the same hash if you swap the operands.
I suggest you leave a comment in the code.

test/hotspot/jtreg/compiler/vectorapi/VectorUnsignedMinMaxOperationsTest.java line 26:

> 24: /**
> 25: * @test
> 26: * @bug 8338201

Bug number does not match this RFE

test/hotspot/jtreg/compiler/vectorapi/VectorUnsignedMinMaxOperationsTest.java line 29:

> 27: * @summary Support new unsigned and saturating vector operators in VectorAPI
> 28: * @modules jdk.incubator.vector
> 29: * @requires vm.compiler2.enabled

I think you can drop that requirement.

test/hotspot/jtreg/compiler/vectorapi/VectorUnsignedMinMaxOperationsTest.java line 109:

> 107: 
> 108:     @Test
> 109:     @IR(counts = {IRNode.UMAX_VB, " >0 "}, applyIf = {"UseAVX", " >0 "})

I think we usually use CPU features, like `avx`.

test/hotspot/jtreg/compiler/vectorapi/VectorUnsignedMinMaxOperationsTest.java line 116:

> 114:                      .lanewise(VectorOperators.UMAX,
> 115:                                ByteVector.fromArray(bspec, byte_in2, i))
> 116:                      .intoArray(byte_out, i);

Suggestion:

            ByteVector.fromArray(bspec, byte_in1, i)
                      .lanewise(VectorOperators.UMAX,
                                ByteVector.fromArray(bspec, byte_in2, i))
                      .intoArray(byte_out, i);

Alignment looked off

test/hotspot/jtreg/compiler/vectorapi/VectorUnsignedMinMaxOperationsTest.java line 453:

> 451:             IntVector vec1 = IntVector.fromArray(ispec, int_in1, i);
> 452:             IntVector vec2 = IntVector.fromArray(ispec, int_in2, i);
> 453:             // UMinV (UMinV vec1, vec2) (UMaxV vec1, vec2) => UMinV vec1 vec2

I think you now always have pattern `minmax (minmax vec1, vec2) (minmax vec1, vec2)`, i.e. with `vec1` as first operand and `vec2` as second. Would be nice to have one where they are swapped:
`minmax (minmax vec1, vec2) (minmax vec2, vec1)`

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21604#pullrequestreview-2536521746
PR Review Comment: https://git.openjdk.org/jdk/pull/21604#discussion_r1906839550
PR Review Comment: https://git.openjdk.org/jdk/pull/21604#discussion_r1906841096
PR Review Comment: https://git.openjdk.org/jdk/pull/21604#discussion_r1906844139
PR Review Comment: https://git.openjdk.org/jdk/pull/21604#discussion_r1906850212
PR Review Comment: https://git.openjdk.org/jdk/pull/21604#discussion_r1906810532
PR Review Comment: https://git.openjdk.org/jdk/pull/21604#discussion_r1906812099
PR Review Comment: https://git.openjdk.org/jdk/pull/21604#discussion_r1906818097
PR Review Comment: https://git.openjdk.org/jdk/pull/21604#discussion_r1906824451
PR Review Comment: https://git.openjdk.org/jdk/pull/21604#discussion_r1906835049