8364670: Extend JVMCI to express fixed binding #26652

MichaelHaas99 · 2025-08-06T07:33:19Z

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Errors

⚠️ OCA signatory status must be verified
⚠️ The pull request body must not be empty.

Integration blocker

⚠️ Title mismatch between PR and JBS for issue JDK-8364670

Issue

JDK-8364670: [JVMCI] Extend JVMCI to express fixed binding (Enhancement - P4) ⚠️ Title mismatch between PR and JBS.

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26652

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

Using diff file

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

bridgekeeper · 2025-08-06T07:34:18Z

Hi @MichaelHaas99, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user MichaelHaas99" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2025-08-06T07:34:51Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-08-06T07:35:45Z

@MichaelHaas99 The following labels will be automatically applied to this pull request:

graal
hotspot

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

tkrodriguez · 2025-08-06T18:23:59Z

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/code/site/Call.java

+    /**
+     * Specifies if the call has a corresponding bytecode.
+     */
+    public final boolean trustBytecode;


I know this is just draft, but I'm wondering what the purpose of this is? It seems like setting bind says whether to do it or not. Is this just for testing?

We should try avoid adding stuff here just for testing if possible.

No not just for testing.
Hotspot assumes that the bound method originated from an underlying bytecode.
Therefore I want sth to say: don't trust the bytecode patch calls in JVMCI compiled code with the bound method. I guess we will need this for the truffle safepoint right?
If we trust it we will follow the normal logic in sharedRuntime.cpp.
Valhalla also expects that if the method is bound and not a method handle then it must have scalarized parameters, see https://github.com/openjdk/valhalla/blob/lworld/src/hotspot/share/runtime/sharedRuntime.cpp#L1244
I think it is better if our logic have as less influence as possible of whatever changes they do in there.
So inserting a truffle safepoint is actually similar to the acmp patch here, except that acmp actually has a corresponding bytecode
https://github.com/openjdk/valhalla/blob/lworld/src/hotspot/share/runtime/sharedRuntime.cpp#L1194

method handle has corresponding bytecode (invoke..), trustBytecode=true
acmp has corresponding bytecode (acmp), trustBytecode=true
truffle safepoint has no corresponding bytecode?, trustBytecode=false

I see. We want to specially identify these call sites as intentionally bound so that we can safely plug into the existing sanity checks and whatever checks are eventually added. I think for the purposes of JVMCI, setting the bind flag is how we indicate this and having a separate flag doesn't make much sense to me. The new reloc info flag could be explicitly_bound which I think is more clear. trust_bytecode implies unrelated things to me.

But that would be a one to one mapping of the explicitly_bound and bind flag then?
As the bind flag means attach the method there is no sense in having an explicitly_bound flag then?

Yes that's what I want to say, also an option yes. What if the underlying bytecode is an invoke itself, can we ensure this will not be the case?

truffle safepoint is no different than the acmp case but we might be at any possible valid bci in a method. So there is a bytecode but we don't know what it is until the safepoint call is inserted in the graph. Hopefully that's not a problem for HotSpot. Are we sure that valhalla doesn't contain any extra fixes to make bound calls at acmp work? I just find it a little hard to believe that somewhere in HotSpot there isn't an assert or two that would be unhappy about the resulting structure.

Our comments are getting ordered weirdly. I agree it might be a little confusing if the call site was an invoke though that seems a little unlikely. We should be getting state afters from FSA. I think we just need to get some code working and see if anything barfs. Or maybe write a better unittest that exercises the possible truffle use case. We could also instrument FSA to report what states and bytecode are assigned to TruffleSafepointNode in a stock Graal build.

truffle safepoint is no different than the acmp case but we might be at any possible valid bci in a method. So there is a bytecode but we don't know what it is until the safepoint call is inserted in the graph. Hopefully that's not a problem for HotSpot. Are we sure that valhalla doesn't contain any extra fixes to make bound calls at acmp work? I just find it a little hard to believe that somewhere in HotSpot there isn't an assert or two that would be unhappy about the resulting structure.

I'll need to have a look maybe, maybe not. But yes let's try it in the jvmcicodeinstaller as well.

Our comments are getting ordered weirdly. I agree it might be a little confusing if the call site was an invoke though that seems a little unlikely. We should be getting state afters from FSA. I think we just need to get some code working and see if anything barfs. Or maybe write a better unittest that exercises the possible truffle use case. We could also instrument FSA to report what states and bytecode are assigned to TruffleSafepointNode in a stock Graal build.

yes sounds good

@tkrodriguez @dougxc
Having thought about it for a while and looked at code in Valhalla, I came up with the following solution for the truffle safepoint:
Calls based on invoke bytecodes are marked as don't reexecute. In Graal an Invoke node is a state split and computes its stateDuring based on the stateAfter. The stateDuring has the StackState After_Pop=!should_reexecute.
As we now have calls that don't correspond to invoke bytecodes, their nodes should rely on a stateBefore (StackState is Before_Pop = should_reexecute) to deoptimize.
In sharedRutime.cpp we can then check if the reexecute bit is set and a method is bound and resolve the call site for the truffle safepoint.
See 21759a9
What do you think?

dougxc · 2025-08-06T18:34:48Z

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/code/site/Call.java

@@ -49,11 +49,27 @@ public final class Call extends Infopoint {
     */
    public final boolean direct;

+    /**
+     * Specifies if the target method should be attached to the call site. This is necessary if the corresponding bytecode does not represent a call or no corresponding bytecode exists.


Suggested change

* Specifies if the target method should be attached to the call site. This is necessary if the corresponding bytecode does not represent a call or no corresponding bytecode exists.

* Specifies if {@link #target} should be attached to the call site. This is necessary if {@link debugInfo} does not represent an invoke bytecode or no corresponding bytecode exists.

Not sure if we should say "an invoke bytecode" if_acmp is no invoke bytecode (Valhalla)

Ok, it would help to mention this example in the comment. And I'm still hoping we can avoid needing to support the "no corresponding bytecode exists" case.

What does no corresponding bytecode exists even mean? The call is in the context of an nmethod and while you don't have to be at an invoke bytecode you must be at some valid bci in an method since a frame state is still required for safepointing.

Corresponding in the sense of although the bytecode is no invoke.. it still has some semantic link (Valhalla acmp)

I think describing those as different cases is confusing for documentation. It seems like it would sufficient to just say This permits the compiler to override the bytecode based call site resolution for cases were special semantics are needed.. Going into more detail than that doesn't seem necessary.

sounds good

tkrodriguez · 2025-08-06T19:24:24Z

We might want to also delete JVMCIRuntime::invoke_static_method_one_arg to make it clear that we are using this new support to replace that helper so we are more loom friendly. That would mean we'd need to have the graal side changes ready to go so we can merge them to galahad too.

tkrodriguez · 2025-08-06T20:08:16Z

The basic shape looks good to me so far, so we just need to confirm if we can use it for truffle or not. Thanks for jumping on this so quickly.

MichaelHaas99 · 2025-08-06T20:22:43Z

Yes I will delete JVMCIRuntime::invoke_static_method_one_arg

Thank you, thanks for the comments, helped a lot.

openjdk · 2025-08-19T18:50:22Z

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

Created JTREG test.

… bytecode.

openjdk · 2025-08-20T12:55:33Z

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

bridgekeeper bot added the oca Needs verification of OCA signatory status label Aug 6, 2025

openjdk bot added graal [email protected] hotspot [email protected] labels Aug 6, 2025

MichaelHaas99 marked this pull request as draft August 6, 2025 11:13

tkrodriguez reviewed Aug 6, 2025

View reviewed changes

dougxc reviewed Aug 6, 2025

View reviewed changes

MichaelHaas99 force-pushed the JDK-8364670_method_binding branch from 5902b64 to a1c3cf7 Compare August 12, 2025 18:16

MichaelHaas99 marked this pull request as ready for review August 18, 2025 13:55

MichaelHaas99 force-pushed the JDK-8364670_method_binding branch from 430f20f to ab184be Compare August 19, 2025 18:49

Michael Haas added 9 commits August 20, 2025 13:57

Provide an API to bind a method over JVMCI.

e44ac2b

Created JTREG test.

Allow JVMCI compiled code to emit a Java call without a corresponding…

8cd03f2

… bytecode.

Rely on the reexecute bit to decide if the JVMCI logic should be used.

db67ff2

Remove trust_bytecode (explicitly_bind) variable.

8071a15

Only add bind boolean for direct calls.

be1292e

Restrict new bind logic to static calls only.

e99b026

Remove dead code.

2dfa055

Updated comment.

b9fb31f

Remove non-static logic again.

61dca0c

MichaelHaas99 force-pushed the JDK-8364670_method_binding branch from ab184be to 61dca0c Compare August 20, 2025 12:53

	* Specifies if the target method should be attached to the call site. This is necessary if the corresponding bytecode does not represent a call or no corresponding bytecode exists.
	* Specifies if {@link #target} should be attached to the call site. This is necessary if {@link debugInfo} does not represent an invoke bytecode or no corresponding bytecode exists.

8364670: Extend JVMCI to express fixed binding #26652

Are you sure you want to change the base?

8364670: Extend JVMCI to express fixed binding #26652

Conversation

MichaelHaas99 commented Aug 6, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Errors

Integration blocker

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Aug 6, 2025

Uh oh!

openjdk bot commented Aug 6, 2025

Uh oh!

openjdk bot commented Aug 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelHaas99 Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelHaas99 Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelHaas99 Aug 6, 2025 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkrodriguez commented Aug 6, 2025

Uh oh!

tkrodriguez commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelHaas99 commented Aug 6, 2025

Uh oh!

openjdk bot commented Aug 19, 2025

Uh oh!

openjdk bot commented Aug 20, 2025

Uh oh!

Uh oh!

MichaelHaas99 commented Aug 6, 2025 •

edited by openjdk bot

Loading

MichaelHaas99 Aug 6, 2025 •

edited

Loading

MichaelHaas99 Aug 12, 2025 •

edited

Loading

MichaelHaas99 Aug 6, 2025 •

edited by bridgekeeper bot

Loading

tkrodriguez commented Aug 6, 2025 •

edited

Loading