Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8270848: Redundant unsafe opmask register allocation in some instruction patterns. #4813

Closed
wants to merge 2 commits into from

Conversation

@jatin-bhateja
Copy link
Member

@jatin-bhateja jatin-bhateja commented Jul 16, 2021

  • Some string compare/array equality patters in x86_[32/64].ad files accept the temporary opmark register operands, instructions using these registers are guarded by target feature checks.
  • Even if control path leading to these instructions is not chosen, RA will still allocate physical registers to these temporary operands.
  • Pulling out the feature checks upto the instruction level can save redundant allocations which may effect spilling decisions.
  • Also existing platform dependent routine Matcher::has_predicated_vector() check the existence of AVX512VL feature which is not required.

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8270848: Redundant unsafe opmask register allocation in some instruction patterns.

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4813/head:pull/4813
$ git checkout pull/4813

Update a local copy of the PR:
$ git checkout pull/4813
$ git pull https://git.openjdk.java.net/jdk pull/4813/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4813

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4813.diff

@jatin-bhateja
Copy link
Member Author

@jatin-bhateja jatin-bhateja commented Jul 16, 2021

/label hotspot-compiler-dev

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 16, 2021

👋 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jul 16, 2021

@jatin-bhateja
The hotspot-compiler label was successfully added.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 16, 2021

Webrevs

Loading

@sviswa7
Copy link

@sviswa7 sviswa7 commented Jul 21, 2021

The patch looks good to me.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jul 21, 2021

@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:

8270848: Redundant unsafe opmask register allocation in some instruction patterns.

Reviewed-by: sviswanathan, kvn

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 166 new commits pushed to the master branch:

  • 89f5c96: 8232066: Remove outdated code/methods from PKIX implementation
  • 9856ace: 8268963: [IR Framework] Some default regexes matching on PrintOptoAssembly in IRNode.java do not work on all platforms
  • b59418f: 8270060: (jdeprscan) tools/jdeprscan/tests/jdk/jdeprscan/TestRelease.java failed with class file for jdk.internal.util.random.RandomSupport not found
  • 4f42eb6: 8269523: runtime/Safepoint/TestAbortOnVMOperationTimeout.java failed when expecting 'VM operation took too long'
  • 77fbd99: 8270341: Test serviceability/dcmd/gc/HeapDumpAllTest.java timed-out
  • 048fb2c: Merge
  • 286d313: 8271489: (doc) Clarify Filter Factory example
  • d09b028: 8271396: Spelling errors
  • 489e5fd: 8268019: C2: assert(no_dead_loop) failed: dead loop detected
  • 6afcf5f: 8270886: Crash in PhaseIdealLoop::verify_strip_mined_scheduling
  • ... and 156 more: https://git.openjdk.java.net/jdk/compare/4927ee426aedbeea0f4119bac0a342c6d3576762...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.

Loading

@openjdk openjdk bot added the ready label Jul 21, 2021
@jatin-bhateja
Copy link
Member Author

@jatin-bhateja jatin-bhateja commented Jul 26, 2021

Thanks @sviswa7 , do we need one more reviewer consent here.

Loading

@jatin-bhateja
Copy link
Member Author

@jatin-bhateja jatin-bhateja commented Jul 28, 2021

Hi @vnkozlov , can you kindly review the patch.

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

From correctness point of view changes are good - we should check features instead of UseAVX flag. As we found hard way, not all avx512 CPUs supports all avx512 features.
But it means that this change is only useful on KNL cpus. Or you have other cases?

Loading

bool ret_value = false;
if (UseAVX > 2) {
ret_value = VM_Version::supports_avx512vl();
return true;
}
return ret_value;
Copy link
Contributor

@vnkozlov vnkozlov Jul 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simplify this code by return UseAVX > 2;. On other hand, can you use some related feature check as you did in .ad files? May be supports_evex ()? Is it enough to check that vector masking is supported?

Loading

@jatin-bhateja
Copy link
Member Author

@jatin-bhateja jatin-bhateja commented Jul 30, 2021

From correctness point of view changes are good - we should check features instead of UseAVX flag. As we found hard way, not all avx512 CPUs supports all avx512 features.
But it means that this change is only useful on KNL cpus. Or you have other cases?

Hi @vnkozlov
Problem will mainly be seen on KNL since they lack VL feature, thus Matcher::has_predicate_register will return a false, but for these patterns opmaks argument will still get allocated. Pulling the feature check out up till the predicate level will guide the matcher to select appropriate patterns without opmask operands for KNL.

Your suggested changes have been incorporated.

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Looks good.
Let me test it before you push.

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Jul 30, 2021

Testing passed clean.

Loading

@jatin-bhateja
Copy link
Member Author

@jatin-bhateja jatin-bhateja commented Jul 30, 2021

/integrate

Loading

@jatin-bhateja
Copy link
Member Author

@jatin-bhateja jatin-bhateja commented Jul 30, 2021

Looks good.
Let me test it before you push.

Thanks for your help!

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jul 30, 2021

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

  • 6c68ce2: 8270947: AArch64: C1: use zero_words to initialize all objects
  • cd7e30e: 8271242: Add Arena regression tests
  • 5b3c418: 8270321: Startup regressions in 18-b5 caused by JDK-8266310
  • baf7797: 8049301: Suspicious use of string identity checks in JComponent.setUIProperty
  • 89f5c96: 8232066: Remove outdated code/methods from PKIX implementation
  • 9856ace: 8268963: [IR Framework] Some default regexes matching on PrintOptoAssembly in IRNode.java do not work on all platforms
  • b59418f: 8270060: (jdeprscan) tools/jdeprscan/tests/jdk/jdeprscan/TestRelease.java failed with class file for jdk.internal.util.random.RandomSupport not found
  • 4f42eb6: 8269523: runtime/Safepoint/TestAbortOnVMOperationTimeout.java failed when expecting 'VM operation took too long'
  • 77fbd99: 8270341: Test serviceability/dcmd/gc/HeapDumpAllTest.java timed-out
  • 048fb2c: Merge
  • ... and 160 more: https://git.openjdk.java.net/jdk/compare/4927ee426aedbeea0f4119bac0a342c6d3576762...master

Your commit was automatically rebased without conflicts.

Loading

@openjdk openjdk bot closed this Jul 30, 2021
@openjdk openjdk bot added integrated and removed ready labels Jul 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 30, 2021

@jatin-bhateja Pushed as commit 71ca0c0.

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

Loading

@openjdk openjdk bot removed the rfr label Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants