RFR: 8342393: Promote commutative vector IR node sharing [v2]

[Vladimir Ivanov]

On Tue, 7 Jan 2025 18:06:50 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> Patch promotes the sharing of commutative vector IR with the same inputs but different input ordering.
>> Unlike scalar IR where we perform edge swapping by [sorting inputs](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/addnode.cpp#L122) based on node indices during IR idealization, for vector IR we chose a simpler approach to decorate commutative operations with a special node-level flag during IR construction thus
>> obviating any dependency on explicit idealization routines. This flag is later used during GVN hashing to enable node sharing.  
>> 
>> Following are the performance stats for JMH micro included with the patch.
>> 
>> 
>> Granite Rapids (P-core Xeon Server)
>> Baseline : 
>> Benchmark                                                                (size)   Mode  Cnt      Score   Error   Units
>> VectorCommutativeOperSharingBenchmark.commutativeByteOperationShairing     1024  thrpt    2   8982.549          ops/ms
>> VectorCommutativeOperSharingBenchmark.commutativeIntOperationShairing      1024  thrpt    2   6072.773          ops/ms
>> VectorCommutativeOperSharingBenchmark.commutativeLongOperationShairing     1024  thrpt    2   2368.856          ops/ms
>> VectorCommutativeOperSharingBenchmark.commutativeShortOperationShairing    1024  thrpt    2  15215.087          ops/ms
>> 
>> Withopt:
>> Benchmark                                                                (size)   Mode  Cnt      Score   Error   Units
>> VectorCommutativeOperSharingBenchmark.commutativeByteOperationShairing     1024  thrpt    2  11963.554          ops/ms
>> VectorCommutativeOperSharingBenchmark.commutativeIntOperationShairing      1024  thrpt    2   7036.088          ops/ms
>> VectorCommutativeOperSharingBenchmark.commutativeLongOperationShairing     1024  thrpt    2   2906.731          ops/ms
>> VectorCommutativeOperSharingBenchmark.commutativeShortOperationShairing    1024  thrpt    2  17148.131          ops/ms
>> 
>> Sierra Forest (E-core Xeon Server)
>> Baseline:
>> Benchmark                                                                (size)   Mode  Cnt     Score   Error   Units
>> VectorCommutativeOperSharingBenchmark.commutativeByteOperationShairing     1024  thrpt    2  2444.359          ops/ms
>> VectorCommutativeOperSharingBenchmark.commutativeIntOperationShairing      1024  thrpt    2  1710.256          ops/ms
>> VectorCommutativeOperSharingBenchmark.commutativeLongOperationShairing     1024  thrpt    2   308.766          ops/ms
>> VectorCommutativeOperSharingBenc...
>
> Jatin Bhateja has updated the pull request incrementally with one additional commit since the last revision:
> 
>   removing spaces

Nice improvement, Jatin.

src/hotspot/share/opto/phaseX.cpp line 144:

> 142:       // node sharing.
> 143:       if (n->is_commutative_operation() && k->in(0) == n->in(0) && req == 3) {
> 144:         if (!((n->in(1) == k->in(1) && n->in(2) == k->in(2)) ||

I find this piece hard to read. What do you think about the following?

if (n->is_commutative_operation()) {
  assert(k->is_commutative_operation(), "");
  assert(req == 3, "");
  if (k->in(0) == n->in(0) &&
      (k->in(1) == n->in(1) || k->in(1) == n->in(2)) &&
      (k->in(2) == n->in(1) || k->in(2) == n->in(2))) {
    // nodes are equal
  } else {
    goto collision;
  }
} else {
 ...

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

> 76:   virtual uint hash() const {
> 77:     if (is_commutative_operation()) {
> 78:       return (uintptr_t)in(1) + (uintptr_t)in(2) + Opcode();

Commutative implies the operation is binary (`req == 3`). Should there be an assert somewhere to ensure it's always the case (ideally, when marking a node as commutative during construction)?

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

PR Review: https://git.openjdk.org/jdk/pull/22863#pullrequestreview-2535334749
PR Review Comment: https://git.openjdk.org/jdk/pull/22863#discussion_r1906032701
PR Review Comment: https://git.openjdk.org/jdk/pull/22863#discussion_r1906020222