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

8265711: C1: Intrinsify Class.getModifier method #3616

Closed
wants to merge 6 commits into from

Conversation

@kelthuzadx
Copy link
Member

@kelthuzadx kelthuzadx commented Apr 22, 2021

It's relatively a common case to get modifiers from a constant Class instance, i.e. ThirdPartyClass.class.getModifiers(). Currently, C1 Canonicalizer missed the opportunity of replacing Class.getModifiers intrinsic calls with compile-time constants.


Progress

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

Issue

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3616

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 22, 2021

👋 Welcome back yyang! 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 openjdk bot added the rfr label Apr 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 22, 2021

@kelthuzadx 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.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 22, 2021

Loading

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Apr 26, 2021

Can I have a review of this PR? Thanks!

Loading

}

public static void main(String... args) {
for (int i = 0; i < 1_0000; i++) {
Copy link
Member

@TobiHartmann TobiHartmann Apr 26, 2021

Choose a reason for hiding this comment

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

The thousand separator is at the wrong place, should be 10_000.

Loading

@@ -548,6 +548,19 @@ void Canonicalizer::do_Intrinsic (Intrinsic* x) {
}
break;
}
case vmIntrinsics::_getModifiers : {
Copy link
Member

@TobiHartmann TobiHartmann Apr 26, 2021

Choose a reason for hiding this comment

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

Although old code below has this weird style, please change this to _getModifiers: {

Loading

InstanceConstant* c = x->argument_at(0)->type()->as_InstanceConstant();
if (c != NULL && !c->value()->is_null_object()) {
ciType* t = c->value()->java_mirror_type();
if (t->is_klass()) {
Copy link
Member

@TobiHartmann TobiHartmann Apr 26, 2021

Choose a reason for hiding this comment

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

Shouldn't t->is_primitive_type() be handled as well?

Loading

Copy link
Member Author

@kelthuzadx kelthuzadx Apr 27, 2021

Choose a reason for hiding this comment

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

Done.

Testing:

  • hotspot/jtreg/compiler

Loading

LIR_Opr temp = new_register(T_METADATA);
// Checking if it's a java mirror of primitive type
__ move(new LIR_Address(receiver.result(), java_lang_Class::klass_offset(), T_ADDRESS), temp, info);
__ cmp(lir_cond_notEqual, temp, LIR_OprFact::metadataConst(0));
Copy link
Member

@TobiHartmann TobiHartmann Apr 27, 2021

Choose a reason for hiding this comment

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

The previous version of your patch was missing the check for a primitive type. Why did testing not catch this?

Also, your PR title is misleading. You are not simply canonicalizing Class.getModifier. You are intrinsifying it.

Loading

Copy link
Member Author

@kelthuzadx kelthuzadx Apr 27, 2021

Choose a reason for hiding this comment

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

Because is_intrinsic_support is not aware of the fact that getModifiers is an intrinsic method. So canonicalizer is even not applied.

Also, your PR title is misleading. You are not simply canonicalizing Class.getModifier. You are intrinsifying it.

Yes, exactly. Changed.

Loading

Copy link
Member Author

@kelthuzadx kelthuzadx Apr 27, 2021

Choose a reason for hiding this comment

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

With the new IR test framework, these kinds of problem would never happen, we can do verification at IR level.

Loading

Copy link
Member

@TobiHartmann TobiHartmann Apr 27, 2021

Choose a reason for hiding this comment

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

Because is_intrinsic_support is not aware of the fact that getModifiers is an intrinsic method. So canonicalizer is even not applied.

But your first version (7f0be4e) already had the change to is_intrinsic_supported? Shouldn't that version have triggered test failures because it does not handle primitive types?

With the new IR test framework, these kinds of problem would never happen, we can do verification at IR level.

Unfortunately not. The IR test framework currently only supports C2.

Loading

Copy link
Member Author

@kelthuzadx kelthuzadx Apr 27, 2021

Choose a reason for hiding this comment

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

But your first version (7f0be4e) already had the change to is_intrinsic_supported? Shouldn't that version have triggered test failures because it does not handle primitive types?

The first version only changes Canonicalizer::do_Intrinsic :_

Loading

Copy link
Member

@TobiHartmann TobiHartmann Apr 27, 2021

Choose a reason for hiding this comment

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

Okay, maybe the force-push screwed up some of the history here, you should try to avoid that (it also makes it much harder to review changes incrementally).

Loading

Copy link
Member Author

@kelthuzadx kelthuzadx Apr 27, 2021

Choose a reason for hiding this comment

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

Okay, maybe the force-push screwed up some of the history here, you should try to avoid that (it also makes it much harder to review changes incrementally).

Got it! (The reason why I forcibly push is that I can not push them normally after merging some commits from master branch.)

Loading

Copy link
Member

@TobiHartmann TobiHartmann Apr 27, 2021

Choose a reason for hiding this comment

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

Even when merging changes from master into your branch, a force-push should not be required. Here's a related discussion:
https://mail.openjdk.java.net/pipermail/hotspot-dev/2020-November/046538.html

Loading

@kelthuzadx kelthuzadx changed the title 8265711: C1: Canonicalize Class.getModifier intrinsic method 8265711: C1: Intrinsify Class.getModifier method Apr 27, 2021
LabelObj* L_not_prim = new LabelObj();
LabelObj* L_done = new LabelObj();

LIR_Opr temp = new_register(T_METADATA);
Copy link
Member

@TobiHartmann TobiHartmann Apr 27, 2021

Choose a reason for hiding this comment

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

temp could be renamed to klass.

Loading

__ branch(lir_cond_always, L_done->label());

__ branch_destination(L_not_prim->label());
__ move(new LIR_Address(receiver.result(), java_lang_Class::klass_offset(), T_ADDRESS), temp, NULL);
Copy link
Member

@TobiHartmann TobiHartmann Apr 27, 2021

Choose a reason for hiding this comment

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

Why are you reloading the klass here?

Also, as @rwestrel explained in the RFR for 8211231, we need to be really careful when emitting branches in C1's LIR.

Loading

Copy link
Member Author

@kelthuzadx kelthuzadx Apr 28, 2021

Choose a reason for hiding this comment

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

Why are you reloading the klass here?

Typo. I forget removing it.

Also, as @rwestrel explained in the RFR for 8211231, we need to be really careful when emitting branches in C1's LIR.

The problem is that there's control flow here that's invisible to
the register allocator. The register allocator only knows about control
flow that connects basic blocks. The control flow here is within a
block. So as far as the register allocator is concerned the code above
is a linear sequence of instructions all of which are all executed

Ok, I see. But from the compiled code, it behaves well:

B1 [0, 0] sux: B0 
__id_Instruction___________________________________________
   0 label [label:0x00007f0c10025f60]
   2 std_entry 

B0 std [0, 4] preds: B1 
__id_Instruction___________________________________________
   8 label [label:0x00007f0c100253c0]
  10 move [Base:[rsi|L] Disp: 16|] [rax|M]  [bci:1]
  12 cmp [NE] [rax|M] [metadata:0x0000000000000000|M] 
  14 branch [NE] [label:0x00007f0c10028d78] 
  16 move [int:1041|I] [rsi|I] 
  18 branch [AL] [label:0x00007f0c10028e18] 
  20 label [label:0x00007f0c10028d78]
  22 move [Base:[rax|M] Disp: 168|I] [rsi|I] 
  24 label [label:0x00007f0c10028e18]
  26 move [rsi|I] [rax|I] 
  28 return [rax|I]  


__bci__use__tid____instr____________________________________
. 0    0     4     B1 [0, 0] -> B0 sux: B0

   0 label [label:0x00007f0c10025f60]
Memory range [0x00007f0ca06a9120..0x00007f0ca06a9120] not readable   2 std_entry 
--------------------------------------------------------------------------------
 ;;  block B1 [0, 0]
  0x00007f0ca06a9120:   mov    %eax,-0x16000(%rsp)
  0x00007f0ca06a9127:   push   %rbp
  0x00007f0ca06a9128:   sub    $0x30,%rsp
--------------------------------------------------------------------------------
__bci__use__tid____instr____________________________________
. 0    0     0     B0 (SV) [0, 4] dom B1 pred: B1

   8 label [label:0x00007f0c100253c0]
Memory range [0x00007f0ca06a912c..0x00007f0ca06a912c] not readable  10 move [Base:[rsi|L] Disp: 16|] [rax|M]  [bci:1]
--------------------------------------------------------------------------------
 ;;  block B1 [0, 0]
  0x00007f0ca06a912c:   mov    0x10(%rsi),%rax
--------------------------------------------------------------------------------
  12 cmp [NE] [rax|M] [metadata:0x0000000000000000|M] 
--------------------------------------------------------------------------------
 ;;  block B1 [0, 0]
  0x00007f0ca06a9130:   cmp    $0x0,%rax
--------------------------------------------------------------------------------
  14 branch [NE] [label:0x00007f0c10028d78] 
--------------------------------------------------------------------------------
 ;;  block B1 [0, 0]
  0x00007f0ca06a9134:   jne    0x00007f0ca06a913a
--------------------------------------------------------------------------------
  16 move [int:1041|I] [rsi|I] 
--------------------------------------------------------------------------------
 ;;  block B1 [0, 0]
  0x00007f0ca06a913a:   mov    $0x411,%esi
--------------------------------------------------------------------------------
  18 branch [AL] [label:0x00007f0c10028e18] 
--------------------------------------------------------------------------------
 ;;  block B1 [0, 0]
  0x00007f0ca06a913f:   jmpq   0x00007f0ca06a9144
--------------------------------------------------------------------------------
  20 label [label:0x00007f0c10028d78]
Memory range [0x00007f0ca06a9144..0x00007f0ca06a9144] not readable  22 move [Base:[rax|M] Disp: 168|I] [rsi|I] 
--------------------------------------------------------------------------------
 ;;  block B1 [0, 0]
  0x00007f0ca06a9144:   mov    0xa8(%rax),%esi
--------------------------------------------------------------------------------
  24 label [label:0x00007f0c10028e18]
Memory range [0x00007f0ca06a914a..0x00007f0ca06a914a] not readable  26 move [rsi|I] [rax|I] 
--------------------------------------------------------------------------------
 ;;  block B1 [0, 0]
  0x00007f0ca06a914a:   mov    %rsi,%rax
--------------------------------------------------------------------------------
  28 return [rax|I]  
--------------------------------------------------------------------------------
 ;;  block B1 [0, 0]
  0x00007f0ca06a914d:   add    $0x30,%rsp
  0x00007f0ca06a9151:   pop    %rbp
  0x00007f0ca06a9152:   cmp    0x118(%r15),%rsp
 ;;  block B0 [0, 4]
  0x00007f0ca06a9159:   ja     0x00007f0ca06a915f
  0x00007f0ca06a915f:   retq   
--------------------------------------------------------------------------------

Also tests under compiler and jdk are passed with slowdebug mode.

Loading

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Looks good to me.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 28, 2021

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

8265711: C1: Intrinsify Class.getModifier method

Reviewed-by: thartmann, 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 330 new commits pushed to the master branch:

  • cd1c17c: 8266404: Fatal error report generated with -XX:+CrashOnOutOfMemoryError should not contain suggestion to submit a bug report
  • 2effdd1: 8267112: JVMCI compiler modules should be kept upgradable
  • da4dfde: 8264777: Overload optimized FileInputStream::readAllBytes
  • 3b11d81: 8266742: Check W^X state on possible safepoint
  • 79b3944: 8266520: Revert to OpenGL as the default 2D rendering pipeline for macOS
  • 3c010a7: 8265705: aarch64: KlassDecodeMovk mode broken
  • cf97252: 8264561: javap get NegativeArraySizeException on bad instruction
  • b8856b1: 8263614: javac allows local variables to be accessed from a static context
  • ea36836: 8267236: Versioned platform link in TestMemberSummary.java
  • d5a15f7: 8263438: Unused method AbstractMemberWriter.isInherited
  • ... and 320 more: https://git.openjdk.java.net/jdk/compare/694acedf1830d16042aceeebcbaf6ced2c95ee2c...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TobiHartmann, @vnkozlov) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

Loading

@openjdk openjdk bot added the ready label Apr 28, 2021
@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Apr 29, 2021

Thank Tobias for reviews!

Loading

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented May 5, 2021

Can I have a second review of this PR?

Thanks,
Yang

Loading

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented May 14, 2021

PING:Can I have a second review of this PR?

Loading

* @summary C1 canonicalizes Foo.class.getModifiers()
* @requires vm.compiler1.enabled
* @library /test/lib
* @run main/othervm -XX:TieredStopAtLevel=1 -Xbatch
Copy link
Contributor

@vnkozlov vnkozlov May 14, 2021

Choose a reason for hiding this comment

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

I would suggest to add 2 additional @run command to make sure test passed in all modes:

  • default: without -XX:TieredStopAtLevel
  • C2 only: with -XX:-TieredCompilation

Loading

Copy link
Member Author

@kelthuzadx kelthuzadx May 17, 2021

Choose a reason for hiding this comment

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

Thank you Vladimir for taking time to look at this. Good suggestion! I've added.

Loading

* @summary Canonicalizes Foo.class.getModifiers() with C2 mode
* @requires vm.compiler2.enabled
* @library /test/lib
* @run main/othervm -XX:TieredStopAtLevel=4 -XX:-TieredCompilation
Copy link
Contributor

@vnkozlov vnkozlov May 17, 2021

Choose a reason for hiding this comment

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

-XX:TieredStopAtLevel=4 does not work when Tiered is off. You don't need this flag.

Loading

Copy link
Member Author

@kelthuzadx kelthuzadx May 18, 2021

Choose a reason for hiding this comment

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

Okay, I learned this from Valhalla project. Maybe it's redundant if -XX:-TieredCompilation is disabled.

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Just one small comment you can fix before push. Otherwise it is good.

Loading

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented May 18, 2021

Thanks Tobias and Vladimir for reviews!

/integrate

Loading

@openjdk openjdk bot added the sponsor label May 18, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 18, 2021

@kelthuzadx
Your change (at version 687b899) is now ready to be sponsored by a Committer.

Loading

@TobiHartmann
Copy link
Member

@TobiHartmann TobiHartmann commented May 18, 2021

/sponsor

Loading

@openjdk
Copy link

@openjdk openjdk bot commented May 18, 2021

@TobiHartmann @kelthuzadx Since your change was applied there have been 332 commits pushed to the master branch:

  • 554caf3: 8251392: Consolidate Metaspace Statistics
  • 3e97b07: 8267116: Lanai: Incorrect AlphaComposite for VolatileImage graphics
  • cd1c17c: 8266404: Fatal error report generated with -XX:+CrashOnOutOfMemoryError should not contain suggestion to submit a bug report
  • 2effdd1: 8267112: JVMCI compiler modules should be kept upgradable
  • da4dfde: 8264777: Overload optimized FileInputStream::readAllBytes
  • 3b11d81: 8266742: Check W^X state on possible safepoint
  • 79b3944: 8266520: Revert to OpenGL as the default 2D rendering pipeline for macOS
  • 3c010a7: 8265705: aarch64: KlassDecodeMovk mode broken
  • cf97252: 8264561: javap get NegativeArraySizeException on bad instruction
  • b8856b1: 8263614: javac allows local variables to be accessed from a static context
  • ... and 322 more: https://git.openjdk.java.net/jdk/compare/694acedf1830d16042aceeebcbaf6ced2c95ee2c...master

Your commit was automatically rebased without conflicts.

Pushed as commit 905b41a.

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

Loading

@kelthuzadx kelthuzadx deleted the canon_opt branch May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants