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
8263582: WB_IsMethodCompilable ignores compiler directives #3195
Conversation
👋 Welcome back chagedorn! A progress list of the required criteria for merging this PR into |
@chhagedorn 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
|
/label add hotspot-compiler |
@chhagedorn |
// returns false regardless of any compiler directives if m was not yet tried to be compiled. The compiler directive ExcludeOption | ||
// to prevent a compilation is evaluated lazily and is only applied when a compilation for m is attempted. | ||
// Another problem is that Method::is_not_compilable() only returns true for CompLevel_any if C1 AND C2 cannot compile it. | ||
// This means that a compilation of m must have been attempted for C1 and C2 before WB::isMethodCompilable(m, CompLevl_any) will |
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.
A typo "CompLevl_any" -> "CompLevel_any".
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, fixed it.
@chhagedorn 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 128 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 |
Thanks Igor for your review! |
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 would like to hear from @neliasso why ExcludeOption is treated so specially ().
Why m->is_not_compilable() returns false
when ExcludeOption is used?
if (comp_level == CompLevel_any) { | ||
// Both compilers could have ExcludeOption set. Check all combinations. | ||
bool excluded_c1 = is_excluded_for_compiler(CompileBroker::compiler1(), mh); | ||
bool excluded_c2 = is_excluded_for_compiler(CompileBroker::compiler2(), mh); |
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.
May be use next instead as we do in WhiteBox::compile_method
at line #992:
AbstractCompiler *comp = CompileBroker::compiler(comp_level);
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.
The problem is that CompileBroker::compiler()
returns NULL
for CompLevel_any
. And even if it returned one compiler, I also need to check the other one to decide if the method is completly non-compilable. That's why I added this additional logic for CompLevel_any
.
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 thought exclude
command does not specify which compilation level (and corresponding compiler) is disabled - it disables all compilations.
But may be it is not true for directives. @neliasso, please, correct me if I am wrong.
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 thought
exclude
command does not specify which compilation level (and corresponding compiler) is disabled - it disables all compilations.
But may be it is not true for directives. @neliasso, please, correct me if I am wrong.
You can set Exclude differently for c1 and c2. That's sometimes very handy when creating more complex combinations of directives.
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.
Changes good then.
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.
if (comp_level == CompLevel_any) { | ||
// Both compilers could have ExcludeOption set. Check all combinations. | ||
bool excluded_c1 = is_excluded_for_compiler(CompileBroker::compiler1(), mh); | ||
bool excluded_c2 = is_excluded_for_compiler(CompileBroker::compiler2(), mh); |
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 thought
exclude
command does not specify which compilation level (and corresponding compiler) is disabled - it disables all compilations.
But may be it is not true for directives. @neliasso, please, correct me if I am wrong.
You can set Exclude differently for c1 and c2. That's sometimes very handy when creating more complex combinations of directives.
if (comp_level == CompLevel_any) { | ||
// Both compilers could have ExcludeOption set. Check all combinations. | ||
bool excluded_c1 = is_excluded_for_compiler(CompileBroker::compiler1(), mh); | ||
bool excluded_c2 = is_excluded_for_compiler(CompileBroker::compiler2(), mh); |
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.
Changes good then.
/integrate |
@chhagedorn Since your change was applied there have been 134 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit ab6faa6. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
While playing around with
WB_IsMethodCompilable
together withcompileonly
I ran into some surprising results for methods that should never be compiled (not part ofcompileonly
):isMethodCompilable
returns true instead of false when such an excluded method was not yet tried to be compiled.The reason for it is that
WB_IsMethodCompilable
directly checksCompilationPolicy::can_be_compiled()
which callsMethod::is_not_compilable()
. However, theExcludeOption
compiler directive is only evaluated lazily upon a compilation attempt. Therefore, if a method was not tried to be compiled, yet,Method::is_not_compilable()
always returns false, regardless of any set compiler directive.I therefore suggest to additionally check the
ExcludeOption
inWB_IsMethodCompilable
. I also cleaned up some wrong use ofCompLevel_any
andCompLevel_all
as suggested by @veresov:CompLevel_any
should only be used to query the state as inis_*()
methods andCompLevel_all
when changing the state is inset_*()
methods.Thanks,
Christian
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3195/head:pull/3195
$ git checkout pull/3195
Update a local copy of the PR:
$ git checkout pull/3195
$ git pull https://git.openjdk.java.net/jdk pull/3195/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 3195
View PR using the GUI difftool:
$ git pr show -t 3195
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3195.diff