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

8275647: Enable multi-register return values for optimized upcall stubs #608

Conversation

JornVernee
Copy link
Member

@JornVernee JornVernee commented Nov 2, 2021

Hi,

After we've added multi-register return support to native invokers, this patch does the same for optimized upcall stubs. The strategy is very similar: the caller (in this case the upcall stub) allocates a return buffer in which the return values are stored, and then after the upcall shuffles the return values from the buffer into the right registers.

This patch also removes the buffered invocation strategy for upcalls, and all related code, such as BufferLayout.

Note, in it's current state this patch will disable regress AArch64 since optimized upcalls are not implemented there yet. i.e. this PR is blocked by: https://bugs.openjdk.java.net/browse/JDK-8275646

Jorn


Progress

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

Issue

  • JDK-8275647: Enable multi-register return values for optimized upcall stubs

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 608

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

Using diff file

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

assert(entry->is_static(), "static only");
// Fill in the signature array, for the calling-convention call.
const int total_out_args = entry->size_of_parameters();
assert(total_out_args > 0, "receiver arg");

BasicType* out_sig_bt = NEW_RESOURCE_ARRAY(BasicType, total_out_args);
BasicType ret_type;
{
int i = 0;
SignatureStream ss(entry->signature());
for (; !ss.at_return_type(); ss.next()) {
out_sig_bt[i++] = ss.type(); // Collect remaining bits of signature
if (ss.type() == T_LONG || ss.type() == T_DOUBLE)
out_sig_bt[i++] = T_VOID; // Longs & doubles take 2 Java slots
}
assert(i == total_out_args, "");
ret_type = ss.type();
}
// skip receiver
BasicType* in_sig_bt = out_sig_bt + 1;
int total_in_args = total_out_args - 1;
Copy link
Member Author

@JornVernee JornVernee Nov 2, 2021

Choose a reason for hiding this comment

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

I've moved this to the shared code in universalUpcallHanlder.cpp, since it can be shared between platforms.

void OptimizedEntryBlob::free(OptimizedEntryBlob* blob) {
assert(blob != nullptr, "caller must check for NULL");
JNIHandles::destroy_global(blob->receiver());
BufferBlob::free(blob);
}
Copy link
Member Author

@JornVernee JornVernee Nov 2, 2021

Choose a reason for hiding this comment

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

We were freeing upcall stubs using CodeCache::free, but this doesn't track memory usage. I've added this method to properly handle freeing upcall stubs.

MethodType highLevelType = callingSequence.methodType();

MethodHandle specializedHandle = target; // initial

// we handle returns first since IMR adds an extra parameter that needs to be specialized as well
if (llReturn != void.class || callingSequence.needsReturnBuffer()) {
Copy link
Member Author

@JornVernee JornVernee Nov 2, 2021

Choose a reason for hiding this comment

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

Note that the old code for handling return values is just moved from below the code that handles arguments (and then handling for mutli-reg returns added to that), since the return buffer argument also needs to be handle along with the rest of arguments.

private record InvocationData(MethodHandle leaf,
Map<VMStorage, Integer> argIndexMap,
Map<VMStorage, Integer> retIndexMap,
CallingSequence callingSequence,
Binding.VMStore[] retMoves,
ABIDescriptor abi) {}
Copy link
Member Author

@JornVernee JornVernee Nov 2, 2021

Choose a reason for hiding this comment

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

Same as with downcalls, I've bundled all the extra arguments into a record.

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 2, 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.

@nick-arm
Copy link
Member

nick-arm commented Nov 4, 2021

Note, in it's current state this patch will disable regress AArch64 since optimized upcalls are not implemented there yet. i.e. this PR is blocked by: https://bugs.openjdk.java.net/browse/JDK-8275646

I've nearly finished this, I'll send the patch tomorrow.

@JornVernee JornVernee force-pushed the Multi_Reg_Returns_Upcalls branch from 5f9f336 to 730379a Compare Nov 9, 2021
@JornVernee JornVernee marked this pull request as ready for review Nov 9, 2021
@openjdk openjdk bot added the rfr Ready for review label Nov 9, 2021
@mlbridge
Copy link

mlbridge bot commented Nov 9, 2021

Webrevs

return layout;
}

const CallRegs ForeignGlobals::parse_call_regs_impl(jobject jconv) const {
Copy link
Member Author

@JornVernee JornVernee Nov 9, 2021

Choose a reason for hiding this comment

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

This is now also moved to shared code.

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Looks very good. I've tested the changes on my machine, and all works fine.

@openjdk
Copy link

openjdk bot commented Nov 16, 2021

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

8275647: Enable multi-register return values for optimized upcall stubs

Reviewed-by: mcimadamore, ngasson

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 96 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 Nov 16, 2021
}

address ProgrammableUpcallHandler::generate_optimized_upcall_stub(jobject mh, Method* entry, jobject jabi, jobject jconv) {
address ProgrammableUpcallHandler::generate_optimized_upcall_stub(jobject receiver, Method* entry,
Copy link
Collaborator

@mcimadamore mcimadamore Nov 16, 2021

Choose a reason for hiding this comment

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

is the name "optimized" still carrying meaning in the new VM code?

Copy link
Member Author

@JornVernee JornVernee Nov 16, 2021

Choose a reason for hiding this comment

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

Not really, but I thought I'd save the renaming for: https://bugs.openjdk.java.net/browse/JDK-8275648

Copy link
Member Author

@JornVernee JornVernee Nov 16, 2021

Choose a reason for hiding this comment

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

Doing it here wouldn't be such a hassle though. Though, there's also OptimizedEntryBlob which requires renaming.

Copy link
Collaborator

@mcimadamore mcimadamore Nov 16, 2021

Choose a reason for hiding this comment

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

That's ok - ship it as is - we can rename later.

Copy link
Member

@nick-arm nick-arm left a comment

AArch64 changes look fine.

@JornVernee
Copy link
Member Author

JornVernee commented Nov 18, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Nov 18, 2021

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

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Nov 18, 2021

@JornVernee Pushed as commit fb07746.

💡 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_Upcalls branch Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
3 participants