8292602: ZGC: C2 late barrier analysis uses invalid dominator information

[robcasloz]

Late ZGC barrier analysis (ZBarrierSetC2::analyze_dominating_barriers()) uses dominator information to elide unnecessary load barriers. This information is invalidated whenever the block ordering phase (PhaseCFG::fixup_flow()) inserts a new block (PhaseCFG::insert_goto_at()).

This changeset updates the dominator information phase after each goto-block insertion in insert_goto_at(), to ensure that late ZGC barrier analysis works correctly. Domination information is encoded in two Block member variables: _idom and _dom_depth. The immediate dominator (_idom) of the new goto-block (block) and its successor (out) are trivially remapped after the insertion of block, whereas the dominator tree depth (_dom_depth) of the out subtree is updated by a depth-first search of the descendants of out.

Additionally, the changeset adds dominator tree checks (PhaseCFG::verify_dominator_tree()) every time PhaseCFG::verify() is called and also at the end of the blockOrdering phase (where insert_goto_at() might be called); and includes dominator information in IGV graphs for ease of debugging.

Alternative Solutions

An alternative solution would be to rebuild the dominator tree before using dominator information in ZGC's late barrier analysis. The alternative has been explored and found to be more complex than this changeset, as PhaseCFG::build_dominator_tree() makes multiple assumptions on the shape of the Ideal graph that do not hold at a later C2 phase.

Another alternative would be to update the dominator tree only if ZGC, the only late consumer of domination information, is enabled. This changeset proposes updating the dominator tree unconditionally for simplicity, uniformity, and better test coverage. The overhead of doing so is insignificant for the main standard benchmark suites (DaCapo, SPECjbb2005, SPECjvm2008, SPECjbb2015, ...). A closer study of the DaCapo benchmarks shows that, on average, insert_goto_at() is called less than once per C2 compilation, and each call traverses less than 1% of the total blocks in the CFG.

Testing

• tier1-3 (windows-x64, linux-x64, linux-aarch64, and macosx-x64; release and debug mode).
• tier4-7 (linux-x64; debug mode).
• fuzzing (~1 h. on each platform).
• verified the dominator trees and node depths of 615 graphs updated by insert_goto_at() by comparing them with the result of an independent dominator construction implementation (based on Python's NetworkX package).

Thanks to Nils Eliasson for finding the issue and providing an initial fix.

---

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-8292602: ZGC: C2 late barrier analysis uses invalid dominator information

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer)
• Tobias Hartmann (@TobiHartmann - Reviewer)

Contributors

• Nils Eliasson <neliasso@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10353

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

Using diff file

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

[bridgekeeper]

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

@robcasloz 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.

[robcasloz]

/label add hotspot-gc

[openjdk]

@robcasloz
The hotspot-gc label was successfully added.

[robcasloz]

/contributor add @neliasso

[openjdk]

@robcasloz
Contributor Nils Eliasson <neliasso@openjdk.org> successfully added.

[mlbridge]

Webrevs

• 00: Full (306ef15e)

[vnkozlov]

Looks good to me.

[openjdk]

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

8292602: ZGC: C2 late barrier analysis uses invalid dominator information

Co-authored-by: Nils Eliasson <neliasso@openjdk.org>
Reviewed-by: kvn, 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 80 new commits pushed to the master branch:

• 83abfa5: 8255670: Improve C2's detection of modified nodes
• 5652030: 8292376: A few Swing methods use inheritDoc on exceptions which are not inherited
• 03f287d: 8293995: Problem list sun/tools/jstatd/TestJstatdRmiPort.java on all platforms because of 8293577
• d5bee4a: 8294086: RISC-V: Cleanup InstructionMark usages in the backend
• 47f233a: 8292202: modules_do is called without Module_lock
• 742bc04: 8294100: RISC-V: Move rt_call and xxx_move from SharedRuntime to MacroAssembler
• 2283c32: 8294149: JMH 1.34 and later requires jopt-simple 5.0.4
• 9f90eb0: 8294062: Improve parsing performance of j.l.c.MethodTypeDesc
• c6be2cd: 8293156: Dcmd VM.classloaders fails to print the full hierarchy
• 711e252: 8294039: Remove "Classpath" exception from java/awt tests
• ... and 70 more: https://git.openjdk.org/jdk/compare/3beca2db0761f8172614bf1b287b694c8595b498...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.

[robcasloz]

Looks good to me.

Thanks for reviewing, Vladimir!

[TobiHartmann]

Looks good to me.

[robcasloz]

Looks good to me.
Thanks for reviewing, Tobias!

[robcasloz]

/integrate

[openjdk]

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

• f6d78cd: 8293657: sun/management/jmxremote/bootstrap/RmiBootstrapTest.java#id1 failed with "SSLHandshakeException: Remote host terminated the handshake"
• a4dc035: 8290910: Wrong memory state is picked in SuperWord::co_locate_pack()
• f3ba332: 8294183: AArch64: Wrong macro check in SharedRuntime::generate_deopt_blob
• df53fa7: 8292328: AccessibleActionsTest.java test instruction for show popup on JLabel did not specify shift key
• 5285035: 8294075: gtest/AsyncLogGtest crashes with SEGV
• 696287d: 8294037: Using alias template to unify hashtables in AsyncLogWriter
• 48cc156: 8293331: Refactor FileDispatcherImpl into operating system-specific components
• f751e60: 8294197: Zero: JVM_handle_linux_signal should not assume deopt NOPs
• 4a6060b: 8294190: Incorrect check messages in SharedRuntime::generate_uncommon_trap_blob
• 0be2b2c: 8292756: java.lang.AssertionError at at jdk.compiler/com.sun.tools.javac.code.Scope$ScopeImpl.leave(Scope.java:386)
• ... and 85 more: https://git.openjdk.org/jdk/compare/3beca2db0761f8172614bf1b287b694c8595b498...master

Your commit was automatically rebased without conflicts.

[openjdk]

@robcasloz Pushed as commit eec992c.

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