-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8360000: RISC-V: implement aot #26101
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
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back mli! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
|
@Hamlin-Li The following label will be automatically applied to this pull request:
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. |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, Thanks for working on enabling this feature.
illegal_instruction(Assembler::csr::time); | ||
emit_int64((uintptr_t)msg); | ||
emit_int64((uintptr_t)str); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that our aarch64 counterpart puts the address of msg into r0
in #24740.
// load msg into r0 so we can access it from the signal handler
// ExternalAddress enables saving and restoring via the code cache
lea(c_rarg0, ExternalAddress((address) str));
And fetches the address from r0
in PosixSignals::pd_hotspot_signal_handler
:
// A pointer to the message will have been placed in `r0`
const char *detail_msg = (const char *)(uc->uc_mcontext.regs[0]);
I am not sure but I guess this is needed for the correct working of aot? Maybe we should do similar things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My test shows that it's fine to keep the current way to pass the stop message to sig handler, in both dump and use time. Seems currently both ways work.
But to keep the consistency with other platforms, I'll change it too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do need to make this change consistent with how it was done for AArch64. The old compiler code embedded the address of the string into the instruction stream immediately after illegal instruction without a relocation. The signal handler used the faulting PC as a way to identify the location of the pointer. That won't work if the code gets saved and loaded as the string may not be at the same address. The new code marks the lea instruction as relocatable and adds the string address to the AOT external strings table. This allows the target of the lea to be relocated when the code is reloaded from the AOT cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. Several minor comments remain after more closer look.
movptr(t1, entry_point, offset, t0); | ||
jalr(t1, offset); | ||
movptr(t1, RuntimeAddress(entry_point), t0); | ||
jalr(t1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This movptr + jalr sequence could be further simplified into rt_call(entry_point)
, which could help save one add instruction.
@@ -4882,7 +4927,7 @@ void MacroAssembler::get_thread(Register thread) { | |||
RegSet::range(x28, x31) + ra - thread; | |||
push_reg(saved_regs, sp); | |||
|
|||
mv(t1, CAST_FROM_FN_PTR(address, Thread::current)); | |||
movptr(t1, ExternalAddress(CAST_FROM_FN_PTR(address, Thread::current))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be more consistent with other places in riscv code where we move an ExternalAddress into a register if we do: la(t1, ExternalAddress(CAST_FROM_FN_PTR(address, Thread::current)))
.
@@ -62,6 +63,11 @@ UncommonTrapBlob* OptoRuntime::generate_uncommon_trap_blob() { | |||
ResourceMark rm; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems more reasonable to move this ResourceMark rm;
closer to its user CodeBuffer buffer(name, 2048, 1024);
.
Similar for other changes in this file and src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp.
@Hamlin-Li @RealFYang I think it might be better to discuss this with on the leyden-dev mailing list before trying to implement the changes needed to match what has been done for AArch64 and x86_64. One good reason for caution is that the Leyden premain project is planning to add further code save/restore capabilities to mainline that have already been prototyped in the Leyden premain branch. So, if you enable AOT code cache initialization for RISCV then you will need to be able/ready to provide all the other parts of the implementation when they arrive. It might be safer to implement what is needed in premain (or in a downstream clone) after discussing both what is needed and why it is needed with the Leyden devs. It would also help if you were to use the testing and benchmark programs that the project is using to check that the aot code cache is working correctly and actually boosting performance. |
@adinn Thank you for the suggestion, I'll check the premain in leyden. |
Hi,
Can you help to review this patch?
https://openjdk.org/jeps/483 introduced aot (class loading/linking) and subsequent prs introduced more features related to it, like preserve adapters(c2i/i2c) and runtime blobs in AOT code cache.
Riscv should support these features and resolve relative issues.
Test
jtreg
run tier1/2/3 and hotspot_cds tests, no new failure found compared to master jdk.
Performance
perf command to run the simplest Hello world java program:
perf data:
Thanks
Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26101/head:pull/26101
$ git checkout pull/26101
Update a local copy of the PR:
$ git checkout pull/26101
$ git pull https://git.openjdk.org/jdk.git pull/26101/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26101
View PR using the GUI difftool:
$ git pr show -t 26101
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26101.diff
Using Webrev
Link to Webrev Comment