-
Notifications
You must be signed in to change notification settings - Fork 6k
8276571: C2: pass compilation options as structure #6237
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 |
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.
I like the way it is going, but unfortunately I find the list of unnamed boolean arguments as confusing and error-prone as before... Could we use "named parameters idiom" here, or some other way to name these parameters?
Something like:
class Options {
Options() : _subsume_loads(false), _do_escape_analysis(false) {};
Options& subsume_loads() { _subsume_loads = true; return *this; }
Options& do_escape_analysis() { _do_escape_analysis = true; return *this; }
}
src/hotspot/share/opto/compile.cpp
Outdated
@@ -487,25 +487,25 @@ CompileWrapper::~CompileWrapper() { | |||
void Compile::print_compile_messages() { | |||
#ifndef PRODUCT | |||
// Check if recompiling | |||
if (_subsume_loads == false && PrintOpto) { | |||
if ((subsume_loads() == false) && PrintOpto) { |
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.
if ((subsume_loads() == false) && PrintOpto) { | |
if (!subsume_loads() && PrintOpto) { |
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.
done
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.
All right, this works too. Next user of Options
would probably have to introduce per-use factory methods to disambiguate constructors, so maybe we could do this early on.
class Options {
static Options for_runtime_stub_gen() const {
return Options(
/* subsume_loads = */ true,
/* do_escape_analysis = */ false,
/* eliminate_boxing = */ false,
/* do_lock_coarsening = */ false,
/* install_code = */ true
);
}
}
@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 35 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 |
Thank you, Aleksey, for review. |
I agree. |
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, Aleksey and Christian. |
/integrate |
Going to push as commit a74a839.
Your commit was automatically rebased without conflicts. |
Currently we pass several compilation options as separate arguments to
Compile
:Originally we had only
subsume_loads
option but we added few since then and we may add more.I suggest to add new
Options
class to pass these values intoCompile
.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6237/head:pull/6237
$ git checkout pull/6237
Update a local copy of the PR:
$ git checkout pull/6237
$ git pull https://git.openjdk.java.net/jdk pull/6237/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 6237
View PR using the GUI difftool:
$ git pr show -t 6237
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6237.diff