8307619: C2 failed: Not monotonic (AndI CastII LShiftI) in TestShiftCastAndNotification.java

[eme64]

The Problem

During CCP, we get to a state like that:

x (int:1)   Phi (int:4)
|           |
|     +-----+
|     |
LShiftI (int:16)
  |
CastII (top)       ConI (int:3)
  |                 |
  +----+  +---------+
       |  |
       AndI

We call AddINode::Value during CCP, and in MulNode::AndIL_shift_and_mask_is_always_zero we uncast both inputs, which leaves us with LShiftI and ConI as the "true" inputs. They both have non-top types, and so we determine that this AndI-LShiftI combination always leads to zero: The Phi has a constant type (int:4). So this leaves the lowest 4 bits zero after the LShiftI. Then and-ing that with int:3 means we extract the lowest 3 bits that are zero. So the result is provably always zero - that is the idea.

Then, we have some type updates (here of x and Phi and LShiftI), and the graph looks like this:

x (int)    Phi (int:0..4)
|           |
|     +-----+
|     |
LShiftI (int)
  |
CastII (top)       ConI (int:3)
  |                 |
  +----+  +---------+
       |  |
       AndI

This leads to shift2 failing to have constant type:

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

Lines 1964 to 1967 in a6b72f5

	const Type* shift2_t = phase->type(shift2);
	if (!shift2_t->isa_int() || !shift2_t->is_int()->is_con()) {
	return false;
	}

And with that, we fall back to MulNode::Value:

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

Lines 559 to 566 in a6b72f5

	const Type* AndINode::Value(PhaseGVN* phase) const {
	// patterns similar to (v << 2) & 3
	if (AndIL_shift_and_mask_is_always_zero(phase, in(1), in(2), T_INT, true)) {
	return TypeInt::ZERO;
	}
	return MulNode::Value(phase);
	}

In MulNode::Value we detect that the CastII has type top, and return top for AndI.

CCP expects the types to become more wide over time, so going from int:0 to top is the wrong direction.

Solution

The problem is with the relatively rare CastII still being top - this seems to be very rare. But the new regression test TestShiftCastAndNotification.java seems to create exactly that case, in combination with -XX:StressCCP.

We should guard against top in one of the AndI inputs inside MulNode::AndIL_shift_and_mask_is_always_zero. This will prevent it from detecting the zero-case, untill MulNode::Value would get a chance to compute a non-top type.

Argument for Solution

Is there still a threat from MulNode::AndIL_shift_and_mask_is_always_zero computing a zero first, and MulNode::Value a type that does not include zero after ward?
As types only widen during CCP, having a zero first means that all inputs now are non-top - in fact they are all T_INT. Since types only widen in the inputs, and a zero combination was possible first, it must also be possible later.

Testing

It used to reproduce with -XX:RepeatCompilation=1000 very quickly, by restricting to that single failing method.
This seems fixed now, I verified it locally.

Passes up to tier5 and stress testing.

---

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-8307619: C2 failed: Not monotonic (AndI CastII LShiftI) in TestShiftCastAndNotification.java

Reviewers

• Christian Hagedorn (@chhagedorn - Reviewer)
• Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13908

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back epeter! 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]

@eme64 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

• 01: Full - Incremental (4963faea)
• 00: Full (cf17d4df)

[chhagedorn]

That looks good to me! As we've discussed offline, I'm also afraid, that there are more such cases where we do not handle top correctly during CCP. Might be worth to further investigate at some point.

[openjdk]

@eme64 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:

8307619: C2 failed: Not monotonic (AndI CastII LShiftI) in TestShiftCastAndNotification.java

Reviewed-by: chagedorn, thartmann

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

• 1a6f981: 8308185: Update Http2TestServerConnection to use SSLSocket.startHandshake()
• b300e73: 8308088: Improve class check in CollectedHeap::is_oop
• e34ecc9: 8296469: Instrument VMError::report with reentrant iteration step for register and stack printing
• 5a92aae: 8308156: VerifyCACerts.java misses blank in error output
• 2f1c654: 8307955: Prefer to PTRACE_GETREGSET instead of PTRACE_GETREGS in method 'ps_proc.c::process_get_lwp_regs'
• d3e5065: 8306460: Clear JVM_ACC_QUEUED flag on methods when dumping dynamic CDS archive
• 563152f: 8308223: failure handler missed jcmd.vm.info command
• cb8b8cd: 8307458: Add periodic heap usage JFR events
• f9a785e: 8307547: Support variant collations
• 4e92991: 8300794: Use @snippet in java.util:i18n
• ... and 100 more: https://git.openjdk.org/jdk/compare/cc396895e5a1dac49f4e341ce91c04b8c092d0af...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.

[TobiHartmann]

Looks good to me.

[eme64]

@chhagedorn @TobiHartmann thanks for the reviews!
/integrate

[openjdk]

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

• eaa80ad: 8300543: Compiler Implementation for Pattern Matching for switch
• 5ccc962: 8308342: Remove MetaspaceClosure::Ref::keep_after_pushing()
• a0f4a94: 8307783: runtime/reflect/ReflectOutOfMemoryError.java timed out
• 939344b: 8304685: Fix whitespace parsing in libjdwp
• 241455f: 8307962: Exclude gc/g1/TestSkipRebuildRemsetPhase.java fails with virtual test thread factory
• 34468e1: 8308021: Update IANA Language Subtag Registry to Version 2023-05-11
• f0aebc8: 8305972: Update XML Security for Java to 3.0.2
• 265f40b: 8308396: Fix offset_of conversion warnings in runtime code
• a5343fa: 8281149: (fs) java/nio/file/FileStore/Basic.java fails with java.lang.RuntimeException: values differ by more than 1GB
• 44218b1: 8308248: Revisit alignment of layout constants on 32-bit platforms
• ... and 148 more: https://git.openjdk.org/jdk/compare/cc396895e5a1dac49f4e341ce91c04b8c092d0af...master

Your commit was automatically rebased without conflicts.

[openjdk]

@eme64 Pushed as commit b6a9f5c.

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