8255120: C2: assert(outer->outcnt() >= phis + 2 && outer->outcnt() <= phis + 2 + stores + 1) failed: only phis

[iwanowww]

JDK-8255000 made post-loopopts IGVN processing predictable, but also reordered it with macro node expansion.

It turned out that macro node expansion is sensitive to narrower types post-loopopts IGVN generates and it either triggers the aforementioned assert or produces less efficient code (some runtime checks aren't eliminated anymore and it adds up some overhead, see JDK-8255705 for details).

I'll file a separate RFE to make macro node expansion more robust (and relax the problematic assert), but for now (jdk16) I propose to restore the original order of transformations.

Testing:

• failing test
• hs-tier1 - hs-tier6
• SPECjvm2008 MPEG
• gc/shenandoah/

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8255120: C2: assert(outer->outcnt() >= phis + 2 && outer->outcnt() <= phis + 2 + stores + 1) failed: only phis

Reviewers

• Tobias Hartmann (@TobiHartmann - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk16 pull/113/head:pull/113
$ git checkout pull/113

[bridgekeeper]

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

@iwanowww The following labels will be automatically applied to this pull request:

• hotspot
• shenandoah

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[iwanowww]

/label remove hotspot
/label add hotspot-compiler

[openjdk]

@iwanowww
The hotspot label was successfully removed.

[openjdk]

@iwanowww
The hotspot-compiler label was successfully added.

[mlbridge]

Webrevs

• 00: Full (5233942)

[TobiHartmann]

Looks good to me. Can JDK-8255705 be closed as duplicate or are there other performance issues left?

[openjdk]

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

8255120: C2: assert(outer->outcnt() >= phis + 2 && outer->outcnt() <= phis + 2 + stores + 1) failed: only phis

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

• 417e1d1: 8259061: C2: assert(found) failed: memory-writing node is not placed in its original loop or an ancestor of it
• 793017d: 8259601: AArch64: Fix reinterpretX2D match rule issue
• 15dd8f3: 8259275: JRuby crashes while resolving invokedynamic instruction
• 1cf2378: 8259353: VectorReinterpretNode is incorrectly optimized out
• 17b4db3: 8259636: Check for buffer backed by shared segment kicks in in unexpected places
• 5f9cd72: 8259645: Revert JDK-8245956 JavaCompiler still uses File API instead of Path API in a specific case
• b03880e: 8259634: MemorySegment::asByteBuffer does not respect spatial bounds
• 8a81cf1: 8259298: broken link in Stream::toList spec

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

[iwanowww]

Thanks, Tobias.

Can JDK-8255705 be closed as duplicate or are there other performance issues left?

I don't have a conclusive answer yet. The fix does improve MPEG score a bit, but it is still slower than jdk-16+20.

jdk-16.latest  150.391 +- 0.361  ops/min
jdk-16.fix     151.212 +- 0.356  ops/min

jdk-16+20      152.267 +- 0.378  ops/min
jdk-16+21      150.343 +- 0.294  ops/min

Will conduct additional performance experiments before deciding what to do with JDK-8255705.

[iwanowww]

/integrate

[openjdk]

@iwanowww Since your change was applied there have been 17 commits pushed to the master branch:

• 5926d75: 8259719: ProblemList runtime/cds/appcds/jigsaw/modulepath/ModulePathAndCP_JFR.java on Windows
• 8abefde: 8259720: ProblemList java/awt/Focus/AppletInitialFocusTest/AppletInitialFocusTest1.java on Windows
• fb8ac24: 8259722: ProblemList two jdk/jfr/startupargs tests on Windows
• ac4cd2e: 8231461: static/instance overload leads to 'unexpected static method found in unbound lookup' when resolving method reference
• 42d2d6d: 8259063: Possible deadlock with vtable/itable creation vs concurrent class unloading
• 6bb6093: 8259657: typo in generated HELP page prevents localization
• 5567530: 8258272: LoadVectorMaskedNode can't be replaced by zero con
• a99df45: 8259560: Zero m68k: "static assertion failed: align" after JDK-8252049
• efc36be: 8258985: Parallel WeakProcessor may use too few threads
• 417e1d1: 8259061: C2: assert(found) failed: memory-writing node is not placed in its original loop or an ancestor of it
• ... and 7 more: https://git.openjdk.java.net/jdk16/compare/67e1b639bafb2efb821de4dc92f9e9c17637fe1f...master

Your commit was automatically rebased without conflicts.

Pushed as commit 0148adf.

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