8288897: Clean up node dump code

[eme64]

I recently did some work in the area of Node::dump and Node::find, see JDK-8287647 and JDK-8283775.

This change sets cleans up the code around, and tries to reduce code duplication.

Things I did:

• remove Node::related. It was added 7 years ago, with JDK-8004073. However, it was not extended to many nodes, and hence it is incomplete, and nobody I know seems to use it.
• refactor dump(int) to use dump_bfs (reduce code duplication).
• redefine categories in dump_bfs, focusing on output types. Mixed type is now also control if it has control output, and memory if it has memory output, etc. Plus, a node is also in the control category if it is_CFG. This makes dump_bfs much more usable, to traverse control and memory flow.
• Other small cleanups, like replacing rarely used dump functions with dump, making removing dead code, make some functions private
• Adding call from debugger comment to VM functions that are useful in debugger
• rename find_node_by_name to find_nodes_by_name and find_node_by_dump to find_nodes_by_dump.
• remove now unused dump indent compiler flag PrintIdealIndentThreshold (notproduct)

---

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-8288897: Clean up node dump code

Reviewers

• Christian Hagedorn (@chhagedorn - Reviewer)
• Xin Liu (@navyxliu - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9234

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

Using diff file

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

[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

• 04: Full - Incremental (975c0e7c)
• 03: Full - Incremental (a95b1260)
• 02: Full - Incremental (b2a2f58f)
• 01: Full - Incremental (1a836616)
• 00: Full (80fe17db)

[chhagedorn]

Otherwise, nice cleanup! I think it's the right thing to remove unused and unmaintained dump methods and reduce code duplication.

Have you checked that the printed node order with dump(X) is the same as before? I'm not sure if that is a strong requirement. I'm just thinking about PrintIdeal with which we do:

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

Line 554 in 17aacde

root()->dump(9999);

Some tools/scripts might depend on the previous order of dump(X). But I'm currently not aware of any such order-dependent processing. For the IR framework, the node order does not matter and if I see that correctly, the dump of an individual node is the same as before. So, it should be fine.

[eme64]

@chhagedorn

Have you checked that the printed node order with dump(X) is the same as before? I'm not sure if that is a strong requirement.

I did try to make sure that the output of dump stays equivalent. As far as I manually inspected, they are. The visit order is the same, and the same nodes are dumped.

[navyxliu]

nice feature!
LGTM. I am not a reviewer. we still needs other reviewers to approve it.

[navyxliu]

@chhagedorn

Have you checked that the printed node order with dump(X) is the same as before? I'm not sure if that is a strong requirement.

I did try to make sure that the output of dump stays equivalent. As far as I manually inspected, they are. The visit order is the same, and the same nodes are dumped.

I also verify that. root()->dump(9999) is still same.
furthermore, I update it with colorful style. it looks pretty cool,huh?

-    root()->dump(9999);
+    tty->print_raw("AFTER: ");
+    tty->print_raw_cr(phase_name);
+    root()->dump_bfs(9999, nullptr, "+#$S");

[navyxliu]

still LGTM. thanks.

[bridgekeeper]

@eme64 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[chhagedorn]

I was out on vacation and only had the chance to have a look it again now. Thanks for doing the updates and verifying that the order is still the same! Thanks @navyxliu for double checking it as well! The colorful dump looks really nice :)

Thanks,
Christian

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

8288897: Clean up node dump code

Reviewed-by: chagedorn, xliu

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

• 0c40128: 7194212: NPE in Flow.visitIdent
• 6eb7c3a: 8290732: JNI DestroyJavaVM can start shutdown when one non-daemon thread remains
• 083e014: 8292233: Increase symtab hash table size
• 45e5b31: 8292244: Remove unnecessary include directories
• 9bfffa0: 8291945: Add OSInfo API for static OS information
• bd58553: 8290833: Remove ConstantPoolCache::walk_entries_for_initialization()
• 755ecf6: 8292153: x86: Represent Registers as values
• dedc05c: 8291640: java/beans/XMLDecoder/8028054/Task.java should use the 3-arg Class.forName
• 3d20a8b: 8291959: FileFontStrike#initNative does not properly initialize IG Table on Windows
• a28ab7b: 8288568: Reduce runtime of java.security microbenchmarks
• ... and 321 more: https://git.openjdk.org/jdk/compare/c7c20661eee727ed8354b19723c359ae7c2d4bd8...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.

[eme64]

Thanks @chhagedorn @navyxliu for the review and comments!
/integrate

[openjdk]

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

• 9833c02: 8292946: GC lock/jni/jnilock001 test failed "assert(gch->gc_cause() == GCCause::_scavenge_alot || !gch->incremental_collection_failed()) failed: Twice in a row"
• d753658: 8293107: GHA: Bump to Ubuntu 22.04
• d4e3e69: 8293344: JDK-8242181 broke stack printing for non-attached threads
• 2c61efe: 8282648: Weaken the InflaterInputStream specification in order to allow faster Zip implementations
• e31c537: 8293224: Add link to openjdk.org/jtreg/ from doc/testing
• 955baa3: 8267374: macOS: Option+Up/Down Arrow don't traverse to beginning/end of line in JTextArea
• 8df671c: 8293355: JDK-8293167 included bad copyright header
• 5bed9f7: 8293290: RISC-V: Explicitly pass a third temp register to MacroAssembler::store_heap_oop
• 48b3ab0: 8293167: Memory leak in JfrThreadSampler if stackdepth is larger than default (64)
• 4067321: 8291586: Broken links in JVMTI specification
• ... and 592 more: https://git.openjdk.org/jdk/compare/c7c20661eee727ed8354b19723c359ae7c2d4bd8...master

Your commit was automatically rebased without conflicts.

[openjdk]

@eme64 Pushed as commit dbb2c4b.

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