-
Notifications
You must be signed in to change notification settings - Fork 5.9k
8337396: Cleanup usage of ExternalAddess #20412
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
Conversation
👋 Welcome back kvn! A progress list of the required criteria for merging this PR into |
@vnkozlov 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:
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 52 new commits pushed to the
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 |
Webrevs
|
Hi, Seems to me that the following two were missed?
|
Thank you, @RealFYang |
Hi Vladimir, If I am following your comment regarding forward exception correctly, it seems that this is being encoded as an ExternalAddress because:
I'm wondering if this can be fixed in the back end. Firstly, can I just clarify that is it just the forward exception target that needs a runtime_call_type reloc? The other uses of TailCallNode and TailJumpNode transfer control from a stub to a return address passed as argument to the call. So, there is no constant supplied as argument and hence nothing to relocate for those cases. If so then could we use a match rule to detect this case e.g.
If we provide this rule with an encoding that loads the target using a runtime reloc then it should be preferred on cost grounds to the loadConP rule which uses the default external reloc. |
I think I found a few more places in aarch64 and x86 where we need to use RuntimeAddress. There may be similar problems in the other arch implementations:
|
Thank you Andrew. Yes, you analysis is correct - we fall into default value when constructing Address() from register. |
I think it is fine to use |
@adinn Can you explain aarch64 code? |
Answering my questions: May be we should also use
|
Actually lea(r, RuntimeAddress a) method generates the same instructions as mov(r, RuntimeAddress a) |
And I got compilation failure for |
I will work on |
I don't think I should work on it in these changes. |
I created RFE JDK-8337702 for TailCall(forward_exception_entry). |
Yes good idea. As you say lea is needed when loading the RuntimeAddress into a register. |
Thank you Andrew for review. |
And some addon change to keep RISC-V part up to date. Manually run workloads with -XX:+VerifyOops -XX:+TraceBytecodes.
|
/contributor add @RealFYang |
@vnkozlov |
Added. Thank you, Fei Yang. |
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.
Looks good.
Thank you, Vladimir |
/integrate |
Going to push as commit 34edc73.
Your commit was automatically rebased without conflicts. |
ExternalAddess
should be used only for data load. For calls (and jump) instructions we should useRuntimeAddress
which usesruntime_call_Relocation
.I found few places where
ExternalAddess
is used incorrectly and fixed them.I also added code to print "hottest" (most referenced)
ExternalAddess
addresses in global table to move them into static global tables which will be introduced by JDK-8334691 and JDK-8337519.Here is current output from debug VM on MacBook M1 (Aarch64):
on MacOS-x64:
and on linux-x64:
Few points about difference in output:
ExternalAddess
or any relocation for messages (strings).stub: forward exception
corresponds toStubRoutines::forward_exception_entry()
for which C2 generates tail-call from C2's stubs. It is difficult to convert it toRuntimeAddress
because how relocation for constants in C2 are handled.dlladdr()
, I used to print C++ symbol name, only works for functions:0x00007f35d5b9c000
points toCompressedOops::_narrow_oop._base
from code in MacroAssembler::verify_heapbase() and on aarch64 verify_heapbase() is empty (guarded by#if 0
).ClassLoader::file_name_for_class_name()+...
on MacOSX-x64 corresponds to strings on linux-x64.Additionally I moved asserts before locks in
ExternalsRecorder
methods.Tested: tier1-3, xcomp, stress
Progress
Issue
Reviewers
Contributors
<fyang@openjdk.org>
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20412/head:pull/20412
$ git checkout pull/20412
Update a local copy of the PR:
$ git checkout pull/20412
$ git pull https://git.openjdk.org/jdk.git pull/20412/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 20412
View PR using the GUI difftool:
$ git pr show -t 20412
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20412.diff
Webrev
Link to Webrev Comment