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
[Compressed Instructions] Support compressed instructions for RVC #7
Conversation
Fixed conflicts with #6 . |
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.
Hello,
I have some questions about this PR:
- Performence of
Assembler::emit()
: this function is called when each intruction is generated, , and instructions like "condition branch" are excuted frequently, but onlybeqz/bnez
have corresponding compressedc.beqz/c.bnez
, maybe part of the call is not needed . - The default value of argument
compressed
is confusing, sometime is true, sometime false. - Naming of compresseed instructions
c_xx
andxx_nc
is confusing, too . - Duplicate code in
Assembler::emit_compressed_ld_st
. - It seems no need to modify
instruction_size
->normale_instruction_size
.
Thanks,
Yanhong
Hi Yanhong, Thanks for your reviews. Your points are undeniable. It is hard to make it an implicit phase base on the currently existing system. Code style is an annoying thing because I have to either hook instructions or fork a piece of code slice but only change one or two of them. Both are tiresome and I chose the former one. Also, especially because of the
Thanks again for your reviews - I know it is not easy to review this patch because it may break the current code style. Regards, |
Hi Yanhong, I have force-pushed my patch. About the questions:
The code style seems better. Thanks for your advice. Would you mind having another review when available? Thanks. Regards, |
…ith relocations (e.g. `INSN_ENTRY_RELOC`) by forcing to use 4-byte instructions to prevent protential problems
Testing on full tiers and no more personal changes for this patch. Thanks, |
… the nmethod entry barrier
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
Gentle ping. Could I get another review? Tests have passed. I can get to merge with JDK17 with this patch entering master. |
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
Hi team,
Could I have a review of this patch of compressed instructions support based on current implementation? Thanks in advance.
This patch can introduce:
Having passed related tests based on the current code base.
There are things about this patch:
Assembler::emit()
and convert them as they can into compressed instructions - it should be because it is somewhat C-Ext's semantics and we cannot change instructions written by programmers explicitly one by one._nc
postfix of some of Assembler instructions: we know a bunch of places should be reserved for patching, where we cannot change them into compressed instructions._nc
is short fornot compressed
- with this, that instruction should keep its origin 4-byte form and remain uncompressed.MachBranchNode
s. Please see the comments in my code - but it seems no potential benefits for compressing theseMachBranchNode
s after we have done some work for this so we can directly disable compression of these instructions until we think of a better plan. We think it is not trivial to support this.This patch may not be a small one so it may take a while to get merged or something - but I was kind of hoping this could be done before the next merge, which contains the biased lock removal and stuff. Hope everything safe.
Thanks again for your great work.
Xiaolin