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

[Emanuel Peter]

On Fri, 28 Mar 2025 09:50:10 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 eight additional commits since the last revision:
> 
>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8342676
>  - Review suggestions incorporated.
>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8342676
>  - 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

@jatin-bhateja Thanks for the updates! I have a few more comments :)

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

> 1043: }
> 1044: 
> 1045: bool VectorNode::is_commutative() {

Why did you make this change? It seems unrelated, maybe it slipped in from another change set? Plus, it is not an accurate name, if `if (in(1)->_idx > in(2)->_idx) {` would fail, you would say that it is not commutative ... which is wrong ;)

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

> 2208:     umax = n->in(2);
> 2209:   }
> 2210:   else if (lopc == Op_UMaxV && ropc == Op_UMinV) {

Suggestion:

  } else if (lopc == Op_UMaxV && ropc == Op_UMinV) {

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

> 2211:     umin = n->in(2);
> 2212:     umax = n->in(1);
> 2213:   }

Suggestion:

  } else {
    // Either both Min or Max.
    return nullptr;
  }

That way, you don't need the check below:
`umin != nullptr && umax != nullptr`.

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

> 2228:   }
> 2229: 
> 2230:   return static_cast<VectorNode*>(n)->VectorNode::Ideal(phase, can_reshape);

Hmm. You now have a function `UMinMaxV_Ideal` that promises to do something specific (i.e. work with a MinMax case). But now you actually call into a more general method `VectorNode::Ideal`. I don't think that is a good approach ;)

Our usual approach is something like this:

  3871 Node *StoreBNode::Ideal(PhaseGVN *phase, bool can_reshape){
  3872   Node *progress = StoreNode::Ideal_masked_input(phase, 0xFF);                                                                                                                                                                                                                                                                                                                                                                       
  3873   if( progress != nullptr ) return progress;
  3874 
  3875   progress = StoreNode::Ideal_sign_extended_input(phase, 24);
  3876   if( progress != nullptr ) return progress;
  3877                                                                 
  3878   // Finally check the default case
  3879   return StoreNode::Ideal(phase, can_reshape);
  3880 }


That way, you don't have to do the whole trick with `static_cast<VectorNode*>(n)->` either.

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21604#pullrequestreview-2725137975
PR Review Comment: https://git.openjdk.org/jdk/pull/21604#discussion_r2018426699
PR Review Comment: https://git.openjdk.org/jdk/pull/21604#discussion_r2018434561
PR Review Comment: https://git.openjdk.org/jdk/pull/21604#discussion_r2018439335
PR Review Comment: https://git.openjdk.org/jdk/pull/21604#discussion_r2018453144