8342393: Promote commutative vector IR node sharing

[jatin-bhateja]

Patch promotes the sharing of commutative vector IR with the same inputs but different input ordering.
Similar to scalar IR where we perform edge swapping by sorting inputs based on node indices during IR idealization.

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
VectorCommutativeOperSharingBenchmark.commutativeShortOperationShairing    1024  thrpt    2  3902.179          ops/ms

Withopt:
Benchmark                                                                (size)   Mode  Cnt     Score   Error   Units
VectorCommutativeOperSharingBenchmark.commutativeByteOperationShairing     1024  thrpt    2  3352.839          ops/ms
VectorCommutativeOperSharingBenchmark.commutativeIntOperationShairing      1024  thrpt    2  2918.805          ops/ms
VectorCommutativeOperSharingBenchmark.commutativeLongOperationShairing     1024  thrpt    2   409.482          ops/ms
VectorCommutativeOperSharingBenchmark.commutativeShortOperationShairing    1024  thrpt    2  6955.057          ops/ms

Please review and share your comments.

Best Regards,
Jatin

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (3 reviews required, with at least 3 Reviewers)

Issue

• JDK-8342393: Promote commutative vector IR node sharing (Enhancement - P4)

Reviewers

• Vladimir Ivanov (@iwanowww - Reviewer)
• Emanuel Peter (@eme64 - Reviewer)
• Sandhya Viswanathan (@sviswa7 - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22863/head:pull/22863
$ git checkout pull/22863

Update a local copy of the PR:
$ git checkout pull/22863
$ git pull https://git.openjdk.org/jdk.git pull/22863/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22863

View PR using the GUI difftool:
$ git pr show -t 22863

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22863.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jbhateja! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@jatin-bhateja This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8342393: Promote commutative vector IR node sharing

Reviewed-by: vlivanov, epeter, sviswanathan

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 22 new commits pushed to the master branch:

• 302bed0: 8350499: Minimal build fails with slowdebug builds
• 0795d11: 8350464: The flags to set the native priority for the VMThread and Java threads need a broader range
• 05b4812: 8350041: Skip test/jdk/java/lang/String/nativeEncoding/StringPlatformChars.java on static JDK
• a891630: 8350480: RISC-V: Relax assertion about registers in C2_MacroAssembler::minmax_fp
• 5cbd9d1: 8349959: Test CR6740048.java passes unexpectedly missing CR6740048.xsd
• 25322aa: 8350258: AArch64: Client build fails after JDK-8347917
• 825ab20: 8350456: Test javax/crypto/CryptoPermissions/InconsistentEntries.java crashed: EXCEPTION_ACCESS_VIOLATION
• 9d9d7a1: 8349759: Add unit test for CertificateBuilder and SimpleOCSPServer test utilities
• b45c32c: 8350287: Cleanup SA's support for CodeBlob subclasses
• bd8ad30: 8350285: Shenandoah: Regression caused by ShenandoahLock under extreme contention
• ... and 12 more: https://git.openjdk.org/jdk/compare/1e87ff01994df16df7de331040fc5d7a4a85f630...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@jatin-bhateja The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 25: Full (acb613da)
• 24: Full - Incremental (316507cb)
• 23: Full - Incremental (fd39a429)
• 22: Full - Incremental (f629a6f0)
• 21: Full - Incremental (cab98719)
• 20: Full - Incremental (12ed8ae8)
• 19: Full - Incremental (05b2655f)
• 18: Full - Incremental (8f457ba4)
• 17: Full - Incremental (4a88fd68)
• 16: Full - Incremental (4b0ff354)
• 15: Full - Incremental (32941f85)
• 14: Full - Incremental (fef09e78)
• 13: Full - Incremental (0d3b538b)
• 12: Full - Incremental (f21e30f1)
• 11: Full - Incremental (a5c35a9d)
• 10: Full - Incremental (d272ac6e)
• 09: Full - Incremental (9fede08b)
• 08: Full - Incremental (99ede6fa)
• 07: Full - Incremental (edb1ced5)
• 06: Full - Incremental (86d0145c)
• 05: Full - Incremental (32919318)
• 04: Full - Incremental (e9be0de1)
• 03: Full - Incremental (39184960)
• 02: Full - Incremental (a4c52ecc)
• 01: Full - Incremental (f42645a1)
• 00: Full (21b2ffd8)

[iwanowww]

Nice improvement, Jatin.

[eme64]

Looks promising, thanks for the work!

[openjdk]

@jatin-bhateja Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[jatin-bhateja]

Hi @eme64, Your comments have been addressed. Kindly verify.

[openjdk]

@jatin-bhateja Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[jatin-bhateja]

Hi @iwanowww , @eme64 , @XiaohongGong , your comments have been addressed.

[eme64]

I have a general question about the approach:
For commutative scalar ops (add, mul, etc), we reorder the edges to allow commoning.

  // Otherwise, sort inputs (commutativity) to help value numbering.
  if( in1->_idx > in2->_idx ) {
    add->swap_edges(1, 2);
    return true;
  }

Have you considered doing the same with vectors?
Or should we rather refactor also the scalars with the check_for_collision, and have a is_commutative_operation for scalar and vector ops?

I'm just afraid that we have now 2 ways of doing things, making things unnecessarily complicated.

Also the basic hash / hash collision functions now have an extra check. Probably the overhead is negligible, but who knows - they used to have goto there for performance... what do you think?

[eme64]

@jatin-bhateja

I discussed it quickly with @TobiHartmann . We think it is nicer if we can do a general refactoring:

• Annotate all commutative nodes with Flag_is_commutative_oper, both for vector and scalar node.
• Apply your idea with checking commutative cases different in find_hash
  
  

For us it is ok if you just apply it basically as is - and file a follow-up RFE so we can refactor away all these:

static bool commute(PhaseGVN* phase, Node* add) {
...
  // Otherwise, sort inputs (commutativity) to help value numbering.
  if( in1->_idx > in2->_idx ) {
    add->swap_edges(1, 2);
    return true;
  }
  return false;
}

We have such swap_edges in a few places, we would have to carefully remove all and test things well.

Alternatively, we first do this refactoring just for the scalar nodes, and then your patch only has to apply it for vector cases.

Since this is touching core Node::hash functionality, I think we need to get @iwanowww to agree on this idea as well.

[eme64]

/reviewers 3 reviewer

Increasing the level so that @TobiHartmann @vnkozlov also sign off on this.

[openjdk]

@eme64
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 3 (with at least 3 Reviewers).

[openjdk]

@jatin-bhateja Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[jatin-bhateja]

Hi @eme64 , I have lowered the feature check to IR annotation for now.

[eme64]

@jatin-bhateja Launched testing for Commit 17 / v22.

[jatin-bhateja]

Hi @eme64 , Kindly share the results of your test runs.

[eme64]

@jatin-bhateja Tests look all good on my side. I'll make another pass in the next few days, and hopefully approve.

[jatin-bhateja]

Hi @eme64 , All comments addressed, looking forward to your approval

[eme64]

@jatin-bhateja Perfect, it looks good now. Let me run testing one more time just to be sure. Please ping me in a day or so for the results!

[jatin-bhateja]

Hi @eme64 , Let us know if its good to land.

[eme64]

Testing passed. Approved. Thanks for the work @jatin-bhateja :)

[sviswa7]

Looks good to me.

[jatin-bhateja]

Hi @TobiHartmann , @iwanowww , Can you kindly approve the patch its waiting for one more approval.

[iwanowww]

Looks even better!

[jatin-bhateja]

/integrate

[openjdk]

Going to push as commit e410af0.
Since your change was applied there have been 24 commits pushed to the master branch:

• f755fad: 8349653: Clarify the docs for MemorySegment::reinterpret
• a5c9a4d: 8349032: C2: Parse Predicate refactoring in Loop Unswitching broke fix for JDK-8290850
• 302bed0: 8350499: Minimal build fails with slowdebug builds
• 0795d11: 8350464: The flags to set the native priority for the VMThread and Java threads need a broader range
• 05b4812: 8350041: Skip test/jdk/java/lang/String/nativeEncoding/StringPlatformChars.java on static JDK
• a891630: 8350480: RISC-V: Relax assertion about registers in C2_MacroAssembler::minmax_fp
• 5cbd9d1: 8349959: Test CR6740048.java passes unexpectedly missing CR6740048.xsd
• 25322aa: 8350258: AArch64: Client build fails after JDK-8347917
• 825ab20: 8350456: Test javax/crypto/CryptoPermissions/InconsistentEntries.java crashed: EXCEPTION_ACCESS_VIOLATION
• 9d9d7a1: 8349759: Add unit test for CertificateBuilder and SimpleOCSPServer test utilities
• ... and 14 more: https://git.openjdk.org/jdk/compare/1e87ff01994df16df7de331040fc5d7a4a85f630...master

Your commit was automatically rebased without conflicts.

[openjdk]

@jatin-bhateja Pushed as commit e410af0.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[jatin-bhateja]

Thanks @eme64 , @iwanowww , @sviswa7 for your approvals.