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.

Sign up for GitHub

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

Jump to bottom

8255903: Enable multi-register return values for native invokers #603

Closed
wants to merge 21 commits into from
Closed

8255903: Enable multi-register return values for native invokers #603

wants to merge 21 commits into from
+734 −934

Conversation

JornVernee
Copy link
Member

@JornVernee JornVernee commented Oct 21, 2021 •
edited by openjdk bot
Loading

Hi,

This patch implements support for multi-register returns in native invokers, and removes the buffered invocation strategy for downcalls. This is achieved, essentially, by using an in memory return: the caller allocates a bit of memory, and the native invoker stub writes the values of the return register to that memory. Then, the post processing code reads the register values back from there.

Currently, the target address of a downcall is handled separately from the other arguments. I initially implemented passing the IMR address the same way, but I realized the removal of the buffered invocation strategy affords us a better way of doing things: we can just make the target address and the IMR address part of the normal calling sequence, and remove a bunch of special-casing code to handle these. We now just use a binding recipe to unbox these arguments and shuffle them into registers, and the ABIDescriptor tells the native invoker stub which registers to pick up these arguments from.

Thanks,
Jorn

Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Issue

  • JDK-8255903: Enable multi-register return values for native invokers

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/panama-foreign pull/603/head:pull/603
$ git checkout pull/603

Update a local copy of the PR:
$ git checkout pull/603
$ git pull https://git.openjdk.java.net/panama-foreign pull/603/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 603

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/panama-foreign/pull/603.diff

Sorry, something went wrong.

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 21, 2021

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

@JornVernee
Remove spurious import

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
b9ae7bf
JornVernee
JornVernee commented Oct 21, 2021
View reviewed changes
src/hotspot/cpu/aarch64/universalNativeInvoker_aarch64.cpp
Comment on lines -223 to -225
assert(!target_uses_register(tmp1->as_VMReg()), "conflict");
assert(!target_uses_register(tmp2->as_VMReg()), "conflict");
assert(!target_uses_register(rthread->as_VMReg()), "conflict");
Copy link
Member Author

Choose a reason for hiding this comment

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

I realized these checks were bogus now that we shuffle arguments as part of the native invoker stub. It is fine for the call to use these registers. Though, if rthread is clobbered by the call, we would need to save and restore it (currently that's not the case on any platform though, so we assume it doesn't happen).

@JornVernee JornVernee marked this pull request as ready for review October 21, 2021 13:23
@openjdk openjdk bot added the rfr Ready for review label Oct 21, 2021
JornVernee
JornVernee commented Oct 21, 2021
View reviewed changes
src/hotspot/share/prims/foreign_globals.cpp
@@ -100,35 +104,10 @@ ForeignGlobals::ForeignGlobals() {
CallConvOffsets.ret_regs_offset = field_offset(k_CC, "retRegs", symVMSArray);
}

int CallRegs::calling_convention(BasicType* sig_bt, VMRegPair *regs, int num_args) const {
Copy link
Member Author

@JornVernee JornVernee Oct 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Since this was essentially the same as DownCallNativeConv after making the target address part of the normal list of arguments, I've removed this implementation of calling_convetion and am now using (DownCall)NativeConv for both down and up calls.

@mlbridge
Copy link

mlbridge bot commented Oct 21, 2021 •
edited
Loading

Webrevs

JornVernee and others added 2 commits October 22, 2021 14:52
@JornVernee
Merge branch 'foreign-memaccess+abi' into Multi_Reg_Returns

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
8e79856
@JornVernee
Fixup all platform stubs
1e30979
mcimadamore
mcimadamore reviewed Oct 27, 2021
View reviewed changes
Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Choose a reason for hiding this comment

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

The changes look good and I like the various simplification. The MH adaptation logic went up in complexity, but this is transient since we're looking into replace combinators with bytecode generation anyway. Overall a good change.

When building and testing the patch I noted one test failure:

TestAarch64CallArranger

It seems varargs related, so I wonder if this patch undoes the fix for 8275332.

@JornVernee
Copy link
Member Author

JornVernee commented Oct 29, 2021 •
edited
Loading

I've just pushed a new iteration that basically renames imr to needsReturnBuffer everywhere (including variants like retBuf* instead of imr*).

The tests failures were a case of the newly added tests needing changes for the changes in this PR (fixed as well).

mcimadamore
mcimadamore approved these changes Oct 29, 2021
View reviewed changes
Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Choose a reason for hiding this comment

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

Looks good

@openjdk
Copy link

openjdk bot commented Oct 29, 2021 •
edited
Loading

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

8255903: Enable multi-register return values for native invokers

Reviewed-by: mcimadamore

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 83 new commits pushed to the foreign-memaccess+abi branch:

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 foreign-memaccess+abi branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Ready to be integrated label Oct 29, 2021
@JornVernee
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Nov 2, 2021

Going to push as commit 818dcfc.
Since your change was applied there have been 83 commits pushed to the foreign-memaccess+abi branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Nov 2, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review labels Nov 2, 2021
@openjdk
Copy link

openjdk bot commented Nov 2, 2021

@JornVernee Pushed as commit 818dcfc.

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

@JornVernee JornVernee deleted the Multi_Reg_Returns branch November 2, 2021 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcimadamore mcimadamore

Assignees
No one assigned
Labels
integrated Pull request has been integrated
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@JornVernee @mcimadamore