8343941: IGV: dump graph at different register allocation steps

[robcasloz]

This changeset dumps C2's low-level intermediate representation at the following intermediate register allocation points:

• Initial liveness: after initial liveness information is computed.
• Aggressive coalescing: after aggressively coalescing live ranges and destructing SSA.
• Initial spilling: after initial round of spilling derived from physical interference graph construction.
• Conservative coalescing: after each round of conservative (colorability-preserving) coalescing (if OptoCoalesce is enabled).
• Iterative spilling: after each round of spilling.
• After iterative spilling: after the main register allocation loop.
• Post-allocation copy removal: after peephole copy removal.
• Merge multiple definitions: after local merging of equivalent nodes related by the same live range.
• Fix up spills: convert load-store spills into memory operand accesses ("CISC spilling") if allowed by the target platform and UseCISCSpill is enabled.

The new dumps have already proved to be useful in the investigation of JDK-8331295.

Testing

• tier1-3 (windows-x64, linux-x64, linux-aarch64, macosx-x64, and macosx-aarch64).
• Tested automatically that dumping, scheduling, and viewing hundreds of the new graphs does not trigger any failure on HotSpot or IGV (by instrumenting IGV to schedule and view graphs eagerly and running java -Xbatch -XX:PrintIdealGraphLevel=4).

---

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-8343941: IGV: dump graph at different register allocation steps (Enhancement - P4)

Reviewers

• Daniel Lundén (@dlunde - Committer) Review applies to e44fa796
• Christian Hagedorn (@chhagedorn - Reviewer)
• Damon Fenacci (@dafedafe - Committer)

Reviewers without OpenJDK IDs

• @abdelhak-zaaim (no known openjdk.org user name / role) Review applies to e44fa796

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22017

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[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 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:

8343941: IGV: dump graph at different register allocation steps

Reviewed-by: chagedorn, dfenacci, dlunden

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

• c977ef7: 8342047: Create Template Assertion Predicates with Halt nodes only instead of uncommon traps
• 23a8c71: 8341790: Fix ExceptionOccurred in java.desktop
• 1e97c1c: 8335989: Implement JEP 494: Module Import Declarations (Second Preview)
• e7d90b9: 8343460: ZGC: Crash in ZRemembered::scan_page_and_clear_remset
• 95a00f8: 8343875: Minor improvements of jpackage test library
• 90e9234: 8344074: RISC-V: C1: More accurate _exception_handler_size and _deopt_handler_size
• 3b28354: 8339288: Improve diagnostic logging runtime/cds/DeterministicDump.java
• 0dab920: 8343984: Fix Unsafe address overflow
• 168b18e: 8343958: Remove security manager impl in java.lang.Process and java.lang.Runtime.exec
• 5ac330b: 8344039: Remove security manager dependency in java.time
• ... and 117 more: https://git.openjdk.org/jdk/compare/c0e6c3b93c0d21debc538e0135805c2957053108...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]

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

[dlunde]

Looks great, and I can confirm the new phases are very useful!

[robcasloz]

Looks great, and I can confirm the new phases are very useful!

Thanks Daniel, feel free to review!

[mlbridge]

Webrevs

• 02: Full - Incremental (fb35674f)
• 01: Full - Incremental (e44fa796)
• 00: Full (90f9a24e)

[chhagedorn]

Looks good!

Just an idea, since you've provided a nice description for each phase in the PR description, should we add these in phasetype.hpp at the phases?

[chhagedorn]

Should we split at the word boundary?

Suggested change

	flags(FIXUP_SPILLS, "Fix up spills") \
	flags(FIX_UP_SPILLS, "Fix up spills") \

[robcasloz]

Thanks, done in commit e44fa79.

[dafedafe]

To be consistent I guess the same could be done for MERGE_MULTIDEFS

[robcasloz]

Thanks Damon, done in commit fb35674.

[robcasloz]

Just an idea, since you've provided a nice description for each phase in the PR description, should we add these in phasetype.hpp at the phases?

I tried this out but could not find a good way to interleave code comments and flags entries (only using multi-line comments with additional backslashes, which looks too convoluted in my opinion).

[chhagedorn]

Just an idea, since you've provided a nice description for each phase in the PR description, should we add these in phasetype.hpp at the phases?

I tried this out but could not find a good way to interleave code comments and flags entries (only using multi-line comments with additional backslashes, which looks too convoluted in my opinion).

I see, that does not seem to be straight forward. I guess then it's okay to omit these descriptions.

[robcasloz]

Thanks Daniel and Christian for reviewing!

[dafedafe]

Very very cool!
@robcasloz do you think it could make sense to add a few IR tests just to make sure that the new steps are actually dumped?

[robcasloz]

Very very cool!

Thanks!

@robcasloz do you think it could make sense to add a few IR tests just to make sure that the new steps are actually dumped?

I think that would be a good idea, but since we do not have any such test yet, I propose to address that in a separate RFE, starting from the most important phase dumps (i.e. in increasing graph dump level). Does that make sense @dafedafe?

[dafedafe]

I think that would be a good idea, but since we do not have any such test yet, I propose to address that in a separate RFE, starting from the most important phase dumps (i.e. in increasing graph dump level). Does that make sense @dafedafe?

Totally! Thanks @robcasloz.

[robcasloz]

Thanks Daniel, Christian, and Damon for reviewing! @chhagedorn may I get a re-approval?

[chhagedorn]

Still good!

[robcasloz]

Thanks Christian!

[robcasloz]

/integrate

[openjdk]

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

• bd6152f: 8343855: HTTP/2 ConnectionWindowUpdateSender may miss some unprocessed DataFrames from closed streams
• c3776db: 8342936: Enhance java.io.IO with parameter-less println() and readln()
• b54bd82: 8344025: Remove unused ISO2022.Encoder.maximumDesignatorLength
• abacece: 8344011: Remove usage of security manager from Class and reflective APIs
• c977ef7: 8342047: Create Template Assertion Predicates with Halt nodes only instead of uncommon traps
• 23a8c71: 8341790: Fix ExceptionOccurred in java.desktop
• 1e97c1c: 8335989: Implement JEP 494: Module Import Declarations (Second Preview)
• e7d90b9: 8343460: ZGC: Crash in ZRemembered::scan_page_and_clear_remset
• 95a00f8: 8343875: Minor improvements of jpackage test library
• 90e9234: 8344074: RISC-V: C1: More accurate _exception_handler_size and _deopt_handler_size
• ... and 121 more: https://git.openjdk.org/jdk/compare/c0e6c3b93c0d21debc538e0135805c2957053108...master

Your commit was automatically rebased without conflicts.

[openjdk]

@robcasloz Pushed as commit a8152bd.

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