8360561: PhaseIdealLoop::create_new_if_for_predicate hits "must be a uct if pattern" assert

[marc-chevalier]

Did you know that ranges can be disjoints and yet not ordered?! Well, in modular arithmetic.

Let's look at a simplistic example:

int x;
if (?) {
    x = -1;
} else {
    x = 1;
}

if (x != 0) {
    return;
}
// Unreachable

With signed ranges, before the second if, x is in [-1, 1]. Which is enough to enter to second if, but not enough to prove you have to enter it: it wrongly seems that after the second if is still reachable. Twaddle!

With unsigned ranges, at this point x is in [1, 2^32-1], and then, it is clear that x != 0. This information is used to refine the value of x in the (missing) else-branch, and so, after the if. This is done with simple lattice meet (Hotspot's join): in the else-branch, the possible values of x are the meet of what is was worth before, and the interval in the guard, that is [0, 0]. Thanks to the unsigned range, this is known to be empty (that is bottom, or Hotspot's top). And with a little reduced product, the whole type of x is empty as well. Yet, this information is not used to kill control yet.

This is here the center of the problem: we have a situation such as:

After node 110 CastII is idealized, it is found to be Top, and then the uncommon trap at 129 is replaced by 238 Halt by being value-dead.

Since the control is not killed, the node stay there, eventually making some predicate-related assert fail as a trap is expected under a ParsePredicate.

And that's what this change proposes: when comparing integers with non-ordered ranges, let's see if the unsigned ranges overlap, by computing the meet. If the intersection is empty, then the values can't be equals, without being able to order them. This is new! Without unsigned information for signed integer, either they overlap, or we can order them. Adding modular arithmetic allows to have non-overlapping ranges that are also not ordered.

Let's also notice that 0 is special: it is important bounds are on each side of 0 (or 2^31, the other discontinuity). For instance if x can be 1 or 5, for instance, both the signed and unsigned range will agree on [1, 5] and not be able to prove it's, let's say, 3.

What would there be other ways to treat this problem a bit more generally? The classic solution is not to use intervals all the time: allow small set of values, up to a fixed cardinal (for instance 5 or 10), after which we switch to a range. This is quite easy and handle many cases: it is not that common that it is important for a variable to be equal to one of 10 distinct values, but not anything else in between. A modulo domain would also work along with interval (with a reduced product), but for only two values, or specific cases. That is not very general. A donut domain can also be helpful, but it needs smart heuristic. For 2 points, there are two donuts: in the previous example [1, 5] and [INT_MIN, INT_MAX] \ [2, 4], but only the second allows to prove 3 is not in. Having signed and unsigned range is somewhat having both donuts for some cases, and having just one when they agree.

There is another underlying question: why do we need to have code both for meet (HS's join, to refine the value of x), and for guarding (to know whether a branch is taken). A typical abstract interpreter would actually do that with just one step, using a Guard function that refines a abstract state given a condition to satisfy. The resulting state is whatever enters the branch, already refined. If the branch is impossible, then the state has an empty concretization. This happens typically when one variable (in a non-relational domain) has an empty value (bottom), then the whole abstract state is empty. It can then be optimized into skipping the whole branch. In Hotspot, there are some major differences:

• the evaluation of the condition is not monolithically done in the abstract domain, but instead we want abstract value of each node
• at the end, we request the value of a comparison, without knowing which operator we are going to use, so the abstract value needs to specify all the operators that would allow entering the branch: instead of having a refined abstract state, we just know for which comparison operator, the abstract state is not empty.

We could imagine another way of working, returning the refined value of each variable in a condition (using a side table or spamming Cast nodes), for a given BoolNode, without holding the abstract domain by the hand too much. But of course, asking first "for which operators is the comparison non-empty", and then "give me the refined value of this variable for this given operator" leads to duplication of work.

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-8360561: PhaseIdealLoop::create_new_if_for_predicate hits "must be a uct if pattern" assert (Bug - P3)

Reviewers

• Manuel Hässig (@mhaessig - Committer)
• Quan Anh Mai (@merykitty - Committer) Review applies to 717af8de
• Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26504

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26504.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:

8360561: PhaseIdealLoop::create_new_if_for_predicate hits "must be a uct if pattern" assert

Reviewed-by: mhaessig, thartmann, qamai

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

• ae0dac4: 8361613: System.console() should only be available for interactive terminal
• 1988222: 8365829: Multiple definitions of static 'phase_names'
• e916ce8: 8365878: jshell TOOLING's javap should use binary names
• ... and 316 more: https://git.openjdk.org/jdk/compare/af5932efb5e873ef3beb8b8999a5ef3fbd094ff6...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.

[merykitty]

Should these be done for CmpL, CmpU, CmpUL as well?

[marc-chevalier]

@merykitty yes, probably, I was indeed looking into which flavors of Cmp would need something like that, and how hard it'd be to exhibit the problem. It's still a draft, it wasn't quite done 😉

[marc-chevalier]

After looking deeper, it seems that we don't have to do the same change: this crash doesn't seem possible in other situations because of the refinement (leading to Top integers) happening only in specific cases, and especially not for longs. Yet, we surely can do the change, seems correct to me. Also, even if it doesn't solve a crash, it can be beneficial (see the test with longs): it already worked, but now, we can get rid of a path, and the graph is simpler. Overall, we don't have to, but we can and should, so there it is!

[mlbridge]

Webrevs

• 01: Full - Incremental (feef30f0)
• 00: Full (717af8de)

[mhaessig]

Thank you for working on this, @marc-chevalier! And the nice explanation.

The changes look good to me. Only the IR test needs a small fix.

[TobiHartmann]

Nice analysis Marc! The fix looks good to me.

[marc-chevalier]

/integrate

Thanks all for reviews!

[openjdk]

Going to push as commit 1f0dfdb.
Since your change was applied there have been 327 commits pushed to the master branch:

• 0b8ae26: 8365189: Remove LockingMode related code from arm32
• 15601b4: 8366029: Do not add -XX:VerifyArchivedFields by default to CDS tests
• 58e7581: 8365893: test/jdk/java/lang/Thread/virtual/JfrEvents.java failing intermittently
• ... and 324 more: https://git.openjdk.org/jdk/compare/af5932efb5e873ef3beb8b8999a5ef3fbd094ff6...master

Your commit was automatically rebased without conflicts.

[openjdk]

@marc-chevalier Pushed as commit 1f0dfdb.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.