-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
8287418: riscv: Fix correctness issue of MacroAssembler::movptr #8913
Conversation
👋 Welcome back xlinzheng! A progress list of the required criteria for merging this PR into |
@zhengxiaolinX 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. |
/label hotspot-compiler |
@zhengxiaolinX |
@zhengxiaolinX |
Webrevs
|
Assembler::patch(branch + 4, 31, 20, (lower >> 16) & 0xfff); // Addi. target[27:16] ==> branch[31:20] | ||
Assembler::patch(branch + 12, 31, 20, (lower >> 5) & 0x7ff); // Addi. target[15: 5] ==> branch[31:20] | ||
Assembler::patch(branch + 20, 31, 20, lower & 0x1f); // Addi/Jalr/Load. target[ 4: 0] ==> branch[31:20] | ||
Assembler::patch(branch + 4, 31, 20, (lower >> 17) & 0xfff); // Addiw. target[27:16] ==> branch[31:20] |
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.
Addiw
-> Addi
Are these comments still right?
target[27:16] ==> branch[31:20]
target[15: 5] ==> branch[31:20]
target[ 4: 0] ==> branch[31:20]
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.
Oh, thank you. My IDE prevents me from seeing the latter comments (the lines are a bit long). I would change that.
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.
Comments fixed. Would you please have another review of the change?
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 now, thanks.
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.
lgtm
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.
New changes look good, thanks.
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 fine.
This won't be reproduced on RV boards like Unmatched which only implement SV39.
But we should fix this for future RV hardwares which will likely to have SV48.
@zhengxiaolinX 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 104 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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@RealFYang) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
Yes, it won't be reproduced on a physical board for now. /integrate |
@zhengxiaolinX |
/sponsor |
Going to push as commit 447ae00.
Your commit was automatically rebased without conflicts. |
@RealFYang @zhengxiaolinX Pushed as commit 447ae00. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi team,
MacroAssembler::movptr()
is designed to load a 47-bit (unsigned) address constant, ranging[0x0, 0x7FFF_FFFF_FFFF]
, and a special case -1 (the Universe::non_oop_word()
as we know, which is0xFFFF_FFFF_FFFF_FFFF
). The former ones are inside a sv48 address space range[1]. Please note that under sv48 a valid address has the bit 47 equal to 0 in user space, so thatMacroAssembler::movptr()
could cover all cases under sv48. However, when loading an immediate value ranging[0x7FFF_8000_0000, 0x7FFF_FFFF_FFFF]
using it, the results would wrongly become[0xFFFF_7FFF_8000_0000, 0xFFFF_7FFF_FFFF_FFFF]
, which indicates the MSB has polluted high bits in rare cases.MacroAssembler::movptr()
is a composition oflui+addi+slli+addi+slli+addi
, and all of them are signed operations, MIPS alike.Precisely, the first
lui+addi
aims to load the first32-bit
; then theslli+addi
would load the11-bit
; finally the lastslli+addi
is going to load the remaining5-bit
.To deal with this, there are two approaches:
(a) Use an
addiw
to replace the firstaddi
.addiw
has nearly the same semantics asaddi
, but after the operation the result would be sign-extended according to the bit 31. Due to this feature, we could use this to clean up the dirty high bits at all times. This could also handle the (-1) case. However,Assembler::li32()
, which is composed oflui+addiw
, will conflict with the new implementation, needing further adaptations. (Personally I a bit dislike of that)(b) Alike V8's implementation [2], the trick here is it loads only the first 31-bit using
lui+addi
, with a leading 0 as the bit 31. So this one could prevent this issue at the beginning. As a trade-off, we need to shift one another bit because the leading 0 occupies one bit. Also this one could also handle the (-1) case as well after minor adaptations. (I like this one)This problem could be reproduced using
-XX:CompressedClassSpaceBaseAddress=0x7FFFF8000000 -XX:CompressedClassSpaceSize=40M -Xshare:off
with fastdebug build, and on Qemu only, for currently I have no access to hardware that supports sv48, and the kernel Ubuntu[3] relies on is Linux 5.15. The kernel (TIP) would first check if hardware sponsors sv57, if not then fall back to sv48, and so on. It is not until Linux 5.17 that sv48 is supported[4]. So this issue could never be reproduced on my boards. But fortunately Qemu could sponsor this, because one could mmap an address in 48-bit address space even in a user-level Qemu.Tested with
-XX:CompressedClassSpaceBaseAddress=0x7FFFF8000000 -XX:CompressedClassSpaceSize=40M -Xshare:off
(reproducible) on Qemu with hotspot tier1 (we should ignore OOM caused the compressed class space), and other tiers are on the way.Testing sanity hotspot tier1~tier4 (could not reproduce). Tier1 is finished without new failures.
Thanks,
Xiaolin
[1] https://github.com/riscv/riscv-isa-manual/blob/9ec8c0105dbf1492b57f6cafdb90a268628f476a/src/supervisor.tex#L1999-L2006
[2] https://github.com/v8/v8/blob/main/src/codegen/riscv64/assembler-riscv64.cc#L3479-L3495
[3] https://cdimage.ubuntu.com/releases/22.04/release/
[4] https://www.phoronix.com/scan.php?page=news_item&px=Linux-5.17-RISC-V-sv48
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8913/head:pull/8913
$ git checkout pull/8913
Update a local copy of the PR:
$ git checkout pull/8913
$ git pull https://git.openjdk.java.net/jdk pull/8913/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 8913
View PR using the GUI difftool:
$ git pr show -t 8913
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8913.diff