Skip to content

8370077: C2: make Compile::_major_progress a boolean #27912

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

8370077: C2: make Compile::_major_progress a boolean #27912

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
324e431
major_progress as bool
marc-chevalier Oct 17, 2025
96a1bc7
Adapt the comment
marc-chevalier Oct 22, 2025
9e5612a
More comment
marc-chevalier Oct 24, 2025
c0b0bde
put back the OR in restORe
marc-chevalier Oct 26, 2025
a774b90
typoes in comment
marc-chevalier Oct 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/hotspot/share/opto/compile.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ class Compile : public Phase {
bool _merge_stores_phase; // Phase for merging stores, after post loop opts phase.
bool _allow_macro_nodes; // True if we allow creation of macro nodes.

int _major_progress; // Count of something big happening
bool _major_progress; // Count of something big happening
Copy link
Contributor

@fandreuz fandreuz Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the comment should be updated too?

Copy link
Member Author

@marc-chevalier marc-chevalier Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. A suggestion? Maybe "Whether something big happened"?

Copy link
Contributor

@eme64 eme64 Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is kind of scary that there is basically no documentation on this. But it is quite important actually. The current comment is really not very helpful.

My understanding is that the flag is set if the loop-opts data-structures are invalid (or at least there is no guarantee that they are valid). So we need to re-build the loop tree.

If we ever set the flag, we don't continue with more loop-opts, but spin back to IGVN, clean the graph, and maybe come back to a new loop-opts round.

There may be others who have a better understanding / definition though.

Copy link
Member Author

@marc-chevalier marc-chevalier Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's the right place for this kind of comment. It's quite hidden, far from where it's actually useful to know we need to set or check that. I'd say it should rather be on PhaseIdealLoop for instance, or PhaseIdealLoop::optimize, something like that, as a part of a more global overview of how things work.

Copy link
Contributor

@eme64 eme64 Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should just be some documentation around the major_progress family of field/methods. Or at least link from there to where the documentation resides ;)

Copy link
Member Author

@marc-chevalier marc-chevalier Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think anyone is saying we should not have such comments. I just think it's out of scope here and mainly I just don't know enough to write anything useful and correct. But if somebody more knowledgeable in this gives me a patch that adds such documentation, I can sneak it in this PR. Otherwise, it will be another time.

Copy link
Member

@chhagedorn chhagedorn Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely, it should not hold up this PR. It was more meant to be an endorsement that we should definitely add some documentation some point.

Copy link
Contributor

@eme64 eme64 Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'd say just file an RFE, and link to the conversation here. Feel free to assign it to me if you don't want to own it ;)

Copy link
Contributor

@eme64 eme64 Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's ask @vnkozlov , @TobiHartmann and @rwestrel , do any of you have a good definition for the major_progress concept?

Copy link
Member

@dean-long dean-long Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until jdk11, there were comments in the code like this:
"If _major_progress, then more loop optimizations follow"
but those uses of major_progress() have been changed to !post_loop_opts_phase(). So "major progress" seems to imply "expect more loop opts".

bool _inlining_progress; // progress doing incremental inlining?
bool _inlining_incrementally;// Are we doing incremental inlining (post parse)
bool _do_cleanup; // Cleanup is needed before proceeding with incremental inlining
Expand Down Expand Up @@ -583,16 +583,16 @@ class Compile : public Phase {
// Control of this compilation.
int fixed_slots() const { assert(_fixed_slots >= 0, ""); return _fixed_slots; }
void set_fixed_slots(int n) { _fixed_slots = n; }
int major_progress() const { return _major_progress; }
void set_inlining_progress(bool z) { _inlining_progress = z; }
int inlining_progress() const { return _inlining_progress; }
void set_inlining_incrementally(bool z) { _inlining_incrementally = z; }
int inlining_incrementally() const { return _inlining_incrementally; }
void set_do_cleanup(bool z) { _do_cleanup = z; }
int do_cleanup() const { return _do_cleanup; }
void set_major_progress() { _major_progress++; }
void restore_major_progress(int progress) { _major_progress += progress; }
void clear_major_progress() { _major_progress = 0; }
bool major_progress() const { return _major_progress; }
void set_major_progress() { _major_progress = true; }
void set_major_progress(bool progress) { _major_progress = progress; }
void clear_major_progress() { _major_progress = false; }
int max_inline_size() const { return _max_inline_size; }
void set_freq_inline_size(int n) { _freq_inline_size = n; }
int freq_inline_size() const { return _freq_inline_size; }
Expand Down
12 changes: 6 additions & 6 deletions src/hotspot/share/opto/loopnode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4934,7 +4934,7 @@ void PhaseIdealLoop::build_and_optimize() {
bool do_max_unroll = (_mode == LoopOptsMaxUnroll);


int old_progress = C->major_progress();
bool old_progress = C->major_progress();
uint orig_worklist_size = _igvn._worklist.size();

// Reset major-progress flag for the driver's heuristics
Expand Down Expand Up @@ -5094,7 +5094,7 @@ void PhaseIdealLoop::build_and_optimize() {
if (C->failing()) { return; }

if (_verify_only) {
C->restore_major_progress(old_progress);
C->set_major_progress(old_progress);
assert(C->unique() == unique, "verification _mode made Nodes? ? ?");
assert(_igvn._worklist.size() == orig_worklist_size, "shouldn't push anything");
return;
Expand Down Expand Up @@ -5139,7 +5139,7 @@ void PhaseIdealLoop::build_and_optimize() {
#endif

if (skip_loop_opts) {
C->restore_major_progress(old_progress);
C->set_major_progress(old_progress);
return;
}

Expand All @@ -5160,7 +5160,7 @@ void PhaseIdealLoop::build_and_optimize() {
}
}

C->restore_major_progress(old_progress);
C->set_major_progress(old_progress);
return;
}

Expand Down Expand Up @@ -5315,7 +5315,7 @@ void PhaseIdealLoop::print_statistics() {
// Build a verify-only PhaseIdealLoop, and see that it agrees with "this".
void PhaseIdealLoop::verify() const {
ResourceMark rm;
int old_progress = C->major_progress();
bool old_progress = C->major_progress();
bool success = true;

PhaseIdealLoop phase_verify(_igvn, this);
Expand All @@ -5332,7 +5332,7 @@ void PhaseIdealLoop::verify() const {
assert(success, "VerifyLoopOptimizations failed");

// Major progress was cleared by creating a verify version of PhaseIdealLoop.
C->restore_major_progress(old_progress);
C->set_major_progress(old_progress);
}

// Perform a BFS starting at n, through all inputs.
Expand Down