RFR: 8288897: Clean up node dump code

[Christian Hagedorn]

On Wed, 22 Jun 2022 11:38:15 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> I recently did some work in the area of `Node::dump` and `Node::find`, see [JDK-8287647](https://bugs.openjdk.org/browse/JDK-8287647) and [JDK-8283775](https://bugs.openjdk.org/browse/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](https://bugs.openjdk.org/browse/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

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:
https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/17aacde50fb971bc686825772e29f6bfecadabda/src/hotspot/share/opto/compile.cpp*L554__;Iw!!ACWV5N9M2RV99hQ!K471rX2EvXAFlcgguMGGFY55CVA_Ml_yUOe0KOL_0YnJQB9uesmknkArF06I273Kmvn12zHkPVNky5kL6ehsGlNf6T73cgc2QA$ 

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.

src/hotspot/share/opto/node.cpp line 1658:

> 1656: }
> 1657: 
> 1658: void find_node_by_dump(Node* start, const char* pattern) {

Since we now only dump nodes, how about renaming this method to `dump_nodes_by_dump()` and only keep `find_node(s?)_by_dump()` to call it from the debugger? Same for the other changed `find*()` methods.

src/hotspot/share/opto/node.cpp line 2205:

> 2203: }
> 2204: 
> 2205: bool PrintBFS::filter_category(const Node* n, Filter& filter) {

Maybe you could add a method comment that you are not filtering on the category for `Mixed` but actually look at the outputs of it and also consider `is_CFG()`.

src/hotspot/share/opto/node.cpp line 2220:

> 2218:   }
> 2219:   if (filter._other && t->has_category(Type::Category::Other)) {
> 2220:     return true;

Just a suggestion: To make it clear that you are only special casing `Mixed` you could leave the `switch` statement and only do the additional checks for `Mixed`. Since this category check is specific to the filtering of `dump_bfs()` and not something you normally perform on a type, I suggest to move this function to the `Filter` class (if that's possible). This would also require to change the implementation of `has_category()` - if it's too complicated, just leave it as it is. It's fine like that.

src/hotspot/share/opto/node.cpp line 2655:

> 2653: // call from debugger: dump Node's inputs (or outputs if d negative)
> 2654: void Node::dump(int d) const {
> 2655:   dump_bfs(abs(d), nullptr, (d>0) ? "+$" : "-$");

Suggestion:

  dump_bfs(abs(d), nullptr, (d > 0) ? "+$" : "-$");

src/hotspot/share/opto/node.cpp line 2661:

> 2659: // call from debugger: dump Node's control inputs (or outputs if d negative)
> 2660: void Node::dump_ctrl(int d) const {
> 2661:   dump_bfs(abs(d), nullptr, (d>0) ? "+$c" : "-$c");

Suggestion:

  dump_bfs(abs(d), nullptr, (d > 0) ? "+$c" : "-$c");

-------------

Changes requested by chagedorn (Reviewer).

PR: https://git.openjdk.org/jdk/pull/9234