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

[Emanuel Peter]

On Tue, 21 Jan 2025 08:02:34 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:
> 
>   Adding functional verification to test points using Verify.checkEQ

@jatin-bhateja thanks for the test updates!

I have some more comments about the VM changes - I think we can improve the code slightly and make it more understandable :)

And thanks for filing the follow-up RFE for scalar nodes. I did not understand: do you want to work on that or not?

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

> 65: //-----------------------------------------------------------------------------
> 66: 
> 67: bool NodeHash::check_for_collision(const Node* n, const Node* k) const {

Suggestion:

bool NodeHash::have_equivalent_inputs(const Node* n, const Node* k) const {

I think that would be a bit more explicit.

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

> 83:         return true;
> 84:       }
> 85:     }

Suggestion:

    for (uint i = 0; i < req; i++) {
      if (n->in(i) != k->in(i)) { // Different inputs?
        return true;
      }
    }

I know you just copied it, but we should fix formatting if we are touching the code anyway ;)

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

> 94:       }
> 95:     }
> 96:   collision:

I would replace this line with a comment like this:
`// k was not a hit. Find another candidate and try again.`

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

> 112:         k->Opcode() == op ) {   // Same Opcode
> 113:       bool collision = check_for_collision(n, k);
> 114:       if (collision == false && n->cmp(*k)) {  // Check for any special bits

Suggestion:

      if (!collision && n->cmp(*k)) {  // Check for any special bits

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

PR Review: https://git.openjdk.org/jdk/pull/22863#pullrequestreview-2566356512
PR Review Comment: https://git.openjdk.org/jdk/pull/22863#discussion_r1924808334
PR Review Comment: https://git.openjdk.org/jdk/pull/22863#discussion_r1924806948
PR Review Comment: https://git.openjdk.org/jdk/pull/22863#discussion_r1924815384
PR Review Comment: https://git.openjdk.org/jdk/pull/22863#discussion_r1924803521