8370077: C2: make Compile::_major_progress a boolean

[marc-chevalier]

Simply change Compile::_major_progress from int to bool since we are only checking if it's non-zero.

There is one detail, we used to have

void          restore_major_progress(int progress) { _major_progress += progress; }

It is used after some verification code (maybe not only?) that may reset the major progress, using the progress saved before the said code.

It has a weird semantics:

Progress before	Progress after verification	Progress after restore	What would be the assignment semantics
0	0	0	0
1	0	1	1
0	1	1	0 (mismatch!)
1	1	2	1 (same truthiness)

It is rather a or than a restore, and a proper boolean version of that would be

void restore_major_progress(bool progress) { _major_progress = _major_progress || progress; }

but then, I'd argue the name is confusing. It also doesn't fit so well the idea that we just want to be back to the situation before the verification code. I suspect the unsaid assumption, is that the 3rd line (progress clear before, set by verification) is not possible. Anyway, I've tried with this or-semantics, or with a more natural

void set_major_progress(bool progress) { _major_progress = progress; }

that actually restore what we saved. Both pass (tier1-6 + some internal tests). Thus, I prefered the simpler semantics.

Thanks,
Marc

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8370077: C2: make Compile::_major_progress a boolean (Enhancement - P5)

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27912/head:pull/27912
$ git checkout pull/27912

Update a local copy of the PR:
$ git checkout pull/27912
$ git pull https://git.openjdk.org/jdk.git pull/27912/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27912

View PR using the GUI difftool:
$ git pr show -t 27912

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27912.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mchevalier! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@marc-chevalier 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:

8370077: C2: make Compile::_major_progress a boolean

Reviewed-by: kvn

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 106 new commits pushed to the master branch:

• cc9483b: 8366182: Some PKCS11Tests are being skipped when they shouldn't
• 470eedb: 8370511: test/jdk/javax/swing/JSlider/bug4382876.java does not release previously pressed keys
• f73e56e: 8361894: sun/security/krb5/config/native/TestDynamicStore.java ensure that the test is run with sudo
• ... and 103 more: https://git.openjdk.org/jdk/compare/e8e2aadd9ea302b7b448d0fda9d069d3813f31c5...master

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 master branch, type /integrate in a new comment.

[openjdk]

@marc-chevalier The following label will be automatically applied to this pull request:

• hotspot-compiler

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.

[mlbridge]

Webrevs

• 02: Full - Incremental (9e5612a1)
• 01: Full - Incremental (96a1bc7c)
• 00: Full (324e4312)

[fandreuz]

Perhaps the comment should be updated too?

[marc-chevalier]

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

[eme64]

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.

[marc-chevalier]

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.

[eme64]

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 ;)

[marc-chevalier]

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.

[chhagedorn]

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.

[eme64]

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 ;)

[eme64]

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

[dean-long]

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".

[eme64]

Thanks for working on this. I agree that the old restore_major_progress semantics was a little strange, and hard to understand. So good you are trying to simplify.

I know that all your testing passed... but if there was a bug we may not notice purely with testing. There could also be performance regressions, in cases where we then don't continue optimizing because we messed up and set major_progress where it should not be set. Do you think it could make sense to run some performance testing?

[vnkozlov]

Or you can keep temporary (just for testing this PR and remove it before integration) original logic in debug VM to compare result of major_progress().

[marc-chevalier]

@vnkozlov I fear I don't understand what you're suggesting.

I've tried to add in my set_major_progress(bool) an assert to check we are not in the 3rd case, the one where the assignment-semantics and the OR-semantics mismatch (that is with progress parameter (old progress) unset and current _major_progress set). And indeed the assert does not fire in tier1-6+some other internal testing.

[vnkozlov]

@marc-chevalier

Here is what I am proposing to check if functionality is preserved and answer @eme64 concern.

1. make sure _major_progress accessed/updated through accessors methods.
2. add "new" field _old_major_progress
3. Restore old accessors methods but rename them with prefix old_ and use them to update/access _old_major_progress
4. In new major_progress() add assert((_old_major_progress > 0) == _major_progress, "should match"). You can print values if they are not matching.

[vnkozlov]

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

My understanding of major_progress is to mark major change to graph which may invalidate built loop tree and dominators information and requires exit current round of loop optimization (build_and_optimize()) and run IGVN to clean graph before starting next round of loop optimizations.

[dean-long]

It doesn't look safe to me to change the restore_major_progress() behavior in build_and_optimize(). It certainly looks possible to call set_major_progress() in the do_max_unroll case (lines 5146 - 5163) at least.

[eme64]

Given @dean-long and @vnkozlov 's answers, I would suggest something like this:

If major progress is set:
  Marks that the loop tree information (get_ctrl, idom, get_loop, etc) could be invalid, and we need to rebuild the loop tree.
  It also indicates that we have made progress, and so it is likely that we can make even more progress in a next round of loop optimizations.
If major progress is not set:
  Loop tree information is valid.
  If major progress is not set at the end of a loop opts phase, then we can stop loop opts, because we do not expect any further progress if we did more loop ops phases.

Suggestions for improvements?

[marc-chevalier]

I've filed this JDK-8370443 forgot to post.

[marc-chevalier]

There are 2 things:

1. restore_major_progress from addition (or OR) semantics to assignment (@dean-long's concern): I've added

void          set_major_progress(bool progress) { precond(!(!progress && _major_progress)); _major_progress = progress; }

To see if we are ever in the case that the old_progress is false and _major_progress is true. That is the only case where the former OR-semantics is not the same as the new set semantics. It passes tier1-6 + other internal tests. I can replace the assignment with a || (the boolean +) if we still have doubts, but then, it seems tests are not exercising this path.
2. Is the type change correct overall. I've did something as @vnkozlov describes: have side by side the bool and the int version of the major progress, have the methods acts on both at the same time: on the int as it used to, on the bool as I propose here. Add the proposed assert in the getter. I've also made sure to assign both the int and the bool version for the 2 places in compile.cpp that assign _major_progress directly. It passes tier1-3 + other internal tests. This also makes sure there is no observable difference between the += for the int version, and the assignment for the bool version.

Here is what I've tested with:
testing.patch

[chhagedorn]

Thanks for the suggestion above @eme64!

Marks that the loop tree information (get_ctrl, idom, get_loop, etc) could be invalid, and we need to rebuild the loop tree.

Is it really invalid or just not as accurate as it could be but still correct? At least during a major loop optimizations like Loop Peeling etc. we try to keep things right and even recompute broken things when we are done, as for example after Loop Unswitching (which seems unnecessary if we assume things can really be invalid when major progress is set):

jdk/src/hotspot/share/opto/loopUnswitch.cpp

Line 314 in ffcb158

_phase->recompute_dom_depth();

It also indicates that we have made progress, and so it is likely that we can make even more progress in a next round of loop optimizations.

I think "made progress" is somewhat hard to quantify. We could do plenty of progress in IGVN but not decide to do another loop opts round. But we could remove one useless bool in IGVN and decide to set major progress:

jdk/src/hotspot/share/opto/ifnode.cpp

Line 1449 in ffcb158

phase->C->set_major_progress();

Given that, I would rephrase it to something like:

"It also indicates that the graph was changed in a way that is promising to be able to apply more loop optimizations."

It seems that "major progress" is probably the wrong term since it not only indicates a major progress but also a "please apply more loop opts". Another good example for that:

jdk/src/hotspot/share/opto/loopnode.cpp

Lines 5291 to 5298 in ffcb158

	// After that switch predicates off and do more loop optimizations.
	if (!C->major_progress() && (C->parse_predicate_count() > 0)) {
	C->mark_parse_predicate_nodes_useless(_igvn);
	assert(C->parse_predicate_count() == 0, "should be zero now");
	if (TraceLoopOpts) {
	tty->print_cr("PredicatesOff");
	}
	C->set_major_progress();

What do you think?

[eme64]

Is it really invalid or just not as accurate as it could be but still correct?

That is a good question. I'm sure there are places where it is indeed invalid, and not just "not as accurate as it could be". What is an example of "not as accurate as it could be" where the information is not invalid?

I suppose we would have to continue the work on VerifyLoopOptimizations, and see how far we can push that. Currently our verification is very basic, and most of VerifyLoopOptimizations is still commented out, because there are violations somewhere. Maybe we cannot get around doing VerifyLoopOptimizations and the documentation of major_progress together.

Still: it could be worth to at least add some documentation, even if we do not have 100% confidence. We should at least write down what we do think we know, and put a caveat that we are not sure, and need to improve the situation in the future.

[vnkozlov]

Thank you, @marc-chevalier, for confirming that new logic works as old one by running different experiments.

@eme64, I agree with you suggested comment.

@chhagedorn, yes, in most cases setting major_progress indicates that calculated loop information (get_ctrl, idom, get_loop, etc) is invalid anymore and needs to be recalculated. I agree that in some cases we indeed too conservative by setting major_progress. But the only downside is compilation time.

[marc-chevalier]

I've added a comment, also using Christian's input.

2. Is the type change correct overall. I've did something as @vnkozlov describes: have side by side the bool and the int version of the major progress, have the methods acts on both at the same time: on the int as it used to, on the bool as I propose here. Add the proposed assert in the getter. I've also made sure to assign both the int and the bool version for the 2 places in compile.cpp that assign _major_progress directly. It passes tier1-3 + other internal tests. This also makes sure there is no observable difference between the += for the int version, and the assignment for the bool version.

I've pushed that to tier 4-6, with success.

[vnkozlov]

Good.

[dean-long]

I did some experiements, and it looks like we don't hit case 3 in build_and_optimize() because when not verifying, major progress is already set. But I'm concerned about ShenandoahBarrierC2Support::expand(), which clears major progress before calling build_and_optimize().