8295166: IGV: dump graph at more locations #16120

dlunde · 2023-10-10T13:31:00Z

This changeset

adds a number of new graph dumps for IdealGraphVisualizer (IGV):
- Before conditional constant propagation
- After register allocation
- After block ordering
- After peephole optimization
- After post-allocation expansion
- Before and after
  - loop predication
  - loop peeling
  - pre/main/post loops
  - loop unrolling
  - range check elimination
  - loop unswitching
  - partial peeling
  - split if
  - superword
adds support for enumeration of repeated IGV graph dumps.
adjusts IGV print levels to encompass the new graph dumps. The old levels 4 and 5 are now levels 5 and 6. The new level 4 is for loop optimization dumps.

Example phase list screenshots in IGV (first at level 6, second at level 4)

Some notes:

While discussing the above changes, a separate question was brought up by @chhagedorn:

On a separate note, I'm wondering how useful it is to always dump all JFR events when calling print_method(). Should this be revisited again in general?
The new IGV graph dump enumeration enables a number of cleanups. There is now another RFE for IGV cleanup: JDK-8319599.

Testing

Platforms: windows-x64, linux-x64, linux-aarch64, macosx-x64, macosx-aarch64

tier1, tier2, tier3, tier4, tier5.
Check that optimized builds (--with-debug-level optimized) still work.

Platforms: linux-x64

Tested that thousands of graphs are correctly opened and visualized with IGV.

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-8295166: IGV: dump graph at more locations (Enhancement - P4)

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer) ⚠️ Review applies to 07aac1c5
Roberto Castañeda Lozano (@robcasloz - Reviewer)
Christian Hagedorn (@chhagedorn - Reviewer) ⚠️ Review applies to 6f872e5d

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16120

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-10-10T13:32:38Z

👋 Welcome back dlunde! 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 · 2023-10-10T13:35:24Z

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

TobiHartmann

Nice enhancement! Looks good to me overall. Some comments below.

Please add an IGV screenshot of how the new phases look in the phase list.

Could you summarize which of the ideas / proposals from the RFE are not covered? We should file a follow-up RFE for them.

Many of the proposed dump locations are educated guesses. Should we adjust any of them?

They look good to me but let's see what others think.

Are the proposed levels (for PrintIdealGraphLevel) reasonable or should we adjust them? I put the loop optimization dumps at level 4 and adjusted IdealGraphVisualizer/README.md accordingly.

I think that's reasonable.

I put most new calls to print_method/print_method_iter within a NOT_PRODUCT. Is this OK?

Existing calls to print_method are not guarded because the method also commits a JFR event and updates Compile::_latest_stage_start_counter, so I think your new code should behave similar.

And please make sure that you verify that the 'optimized' build (--with-debug-level optimized) still works. It's a level between fastdebug and release where both #ifdef ASSERT and #ifdef PRODUCT are false.

src/hotspot/share/opto/compile.cpp

TobiHartmann · 2023-10-12T07:40:50Z

src/hotspot/share/opto/compile.cpp

+  int iter = ++_igv_phase_iter[cpt];
+  print_method(cpt, level, n, iter);
+#else
+  print_method(cpt, level, n);


This is dead code because all calls are guarded by NOT_PRODUCT, right?

Not quite, there is one unguarded call for PHASE_PHASEIDEALLOOP_ITERATIONS at level 2. This is an existing phase that I converted from print_method to print_method_iter (as suggested by @chhagedorn in the issue corresponding to this PR).

The current rule seems to be that we always want to emit a JFR event when dumping a graph - regardless of whether we dump a graph or not. I think we should follow this convention and remove the NOT_PRODUCT from the calls to print_method_iter().

On a separate note, I'm wondering how useful it is to always dump all events when calling print_method(). Should this be revisited again in general?

I have now incorporated the functionality of print_method_iter directly into print_method, and also removed all the NOT_PRODUCT wrappers.

I'm resolving this thread now, should we move the discussion regarding JFR event dumping somewhere else?

Maybe we can mention it as a side node in the PR description that it is something we should think about at some point. But it should not block this PR.

Sure, I'll add it to the final (non-draft) PR description.

src/utils/IdealGraphVisualizer/.gitignore

dlunde · 2023-10-13T11:21:12Z

Thanks for the review Tobias!

Please add an IGV screenshot of how the new phases look in the phase list.

I've attached two sample screenshots of the phase list (from running one of the tests in hotspot/jtreg/compiler/loopopts/PartialPeelingUnswitch.java). Note the enumeration of the loop optimizations and the attached target loop node indices.

Could you summarize which of the ideas / proposals from the RFE are not covered? We should file a follow-up RFE for them.

I believe I have covered everything in the RFE.

Existing calls to print_method are not guarded because the method also commits a JFR event and updates Compile::_latest_stage_start_counter, so I think your new code should behave similar.

Ah, I see. Note, however, that the existing call to print_method for PHASE_AFTER_ITER_GVN_STEP (at level 4) is guarded. Also, the bytecode printing (level 5) is guarded (although it does not use Compile::print_method).

And please make sure that you verify that the 'optimized' build (--with-debug-level optimized) still works. It's a level between fastdebug and release where both #ifdef ASSERT and #ifdef PRODUCT are false.

I'll make sure to include this when testing.

robcasloz

Hi Daniel, thanks for working on this! The code changes themselves look good, I have a few comments/suggestions:

It might make sense to create a new print level between current 3 and 4 including the new loop transformation dumps but not the individual IGVN step dumps.
I see the value in numbering the PHASE_PHASEIDEALLOOP_ITERATIONS dumps, but I am not sure numbering the new loop transformations is worth the additional complexity (_igv_phase_iter array, etc.). Limiting numbering to PHASE_PHASEIDEALLOOP_ITERATIONS dumps would not require any additional state in Compile.
In my opinion, it would be clearer to only dump loop transformations if they actually take place. I find it a bit confusing to see e.g. Before/After superword ... graph dumps when vectorization has actually failed and the graph has not changed at all. Dumping only after effective transformations would also match better the output of TraceLoopOpts. Enforcing this invariant would require getting rid of the Before... dumps though, but this is acceptable (and perhaps even preferable) in my opinion.

chhagedorn

Thanks for working on this! This is a good addition and helps to better debug with IGV. I left a few comments with some suggestion and improvement ideas. I gave it some more thought to improve some of the places where we dump the phases and how. But this is of course open for discussion.

src/hotspot/share/opto/loopPredicate.cpp

src/hotspot/share/opto/phasetype.hpp

chhagedorn · 2023-11-02T16:46:36Z

src/hotspot/share/opto/compile.cpp

+  int iter = ++_igv_phase_iter[cpt];
+  print_method(cpt, level, n, iter);
+#else
+  print_method(cpt, level, n);


The current rule seems to be that we always want to emit a JFR event when dumping a graph - regardless of whether we dump a graph or not. I think we should follow this convention and remove the NOT_PRODUCT from the calls to print_method_iter().

On a separate note, I'm wondering how useful it is to always dump all events when calling print_method(). Should this be revisited again in general?

src/hotspot/share/opto/phasetype.hpp

chhagedorn · 2023-11-03T12:39:57Z

src/hotspot/share/opto/compile.cpp

+  if (iter > 0) {
+    ss.print(" %d", iter);
+  }


I'm not so sure about having an extra method print_method_iter() where the user need to keep track if a method is possibly repeated or not. I therefore suggest to only keep print_method() with its original signature and do the increment here like this:

int iter = ++_igv_phase_iter[cpt]; if (iter > 1) { ss.print(" %d", iter); }

Doing it this way we only add a number for the second time a phase is dumped again. I guess that's fine. But I'm open for other opinions about that.

I agree with just having a print_method. I initially hesitated to modify print_method in the way you suggest, as it would then add iteration numbering to all phases with no exceptions. But, it seems this is a feature we want then?

I think it could be useful for any repeated phase, if others also agree with that. And we would still keep the same name for phases that are only printed once and only add a number for the repeated dumps.

I've changed it for now. If anyone does not agree, please let us know.

src/hotspot/share/opto/loopTransform.cpp

src/hotspot/share/opto/loopopts.cpp

src/hotspot/share/opto/loopTransform.cpp

src/hotspot/share/opto/loopUnswitch.cpp

src/hotspot/share/opto/superword.cpp

src/hotspot/share/opto/loopPredicate.cpp

src/hotspot/share/opto/phasetype.hpp

…hases

src/hotspot/share/opto/compile.cpp

chhagedorn

Otherwise, it looks good to me! Thanks for addressing all the comments and suggestions.

src/hotspot/share/opto/loopopts.cpp

src/hotspot/share/opto/phasetype.hpp

test/hotspot/jtreg/compiler/lib/ir_framework/CompilePhase.java

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

robcasloz

Thanks for addressing all the comments and thanks for this nice contribution! The additional dumps, in combination with the CFG view in IGV, make C2 loop optimizations much more approachable.

dlunde · 2023-12-08T10:57:38Z

Thanks for the comments and reviews! The tests are now finished, integrating (and I need a sponsor).

/integrate

openjdk · 2023-12-08T10:58:46Z

@dlunde
Your change (at version 61be342) is now ready to be sponsored by a Committer.

robcasloz · 2023-12-08T11:06:50Z

/sponsor

openjdk · 2023-12-08T11:08:11Z

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

9e48b90: 8310524: C2: record parser-generated LoadN nodes for IGVN
bad5edf: 8320959: jdk/jfr/event/runtime/TestShutdownEvent.java crash with CONF=fastdebug -Xcomp
f577385: 8316738: java/net/httpclient/HttpClientLocalAddrTest.java failed in timeout
86623aa: 8320786: Remove ThreadGroup.stop
af5c492: 8320532: Remove Thread/ThreadGroup suspend/resume
cb7e3d2: 8321560: [BACKOUT] 8299426: Heap dump does not contain virtual Thread stack references
25dc476: 8286827: BogusColorSpace methods return wrong array
11e4a92: 8320597: RSA signature verification fails on signed data that does not encode params correctly
354ea4c: 8299426: Heap dump does not contain virtual Thread stack references
959a443: 8288712: Typo in javadoc in javax.imageio.ImageReader.java
... and 71 more: https://git.openjdk.org/jdk/compare/1cf7ef520b73321c9fe7856b2f55ca6ecb555126...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-12-08T11:08:18Z

@robcasloz @dlunde Pushed as commit 701bc3b.

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

openjdk bot changed the title ~~8295166~~ 8295166: IGV: dump graph at more locations Oct 10, 2023

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Oct 10, 2023

TobiHartmann reviewed Oct 12, 2023

View reviewed changes

robcasloz reviewed Oct 18, 2023

View reviewed changes

chhagedorn reviewed Nov 3, 2023

View reviewed changes

dlunde force-pushed the 8295166-igv branch 6 times, most recently from 2e763a2 to b80215f Compare November 7, 2023 16:15

chhagedorn reviewed Nov 8, 2023

View reviewed changes

src/hotspot/share/opto/loopTransform.cpp Outdated Show resolved Hide resolved

src/hotspot/share/opto/loopUnswitch.cpp Outdated Show resolved Hide resolved

src/hotspot/share/opto/superword.cpp Outdated Show resolved Hide resolved

chhagedorn reviewed Nov 8, 2023

View reviewed changes

src/hotspot/share/opto/loopPredicate.cpp Show resolved Hide resolved

chhagedorn reviewed Nov 8, 2023

View reviewed changes

src/hotspot/share/opto/phasetype.hpp Outdated Show resolved Hide resolved

dlunde force-pushed the 8295166-igv branch from c4ab4a4 to 5c98ce7 Compare December 4, 2023 10:47

dlunde added 12 commits December 5, 2023 13:04

Add register allocation IGV dump

8f0378e

Add more IGV dumps

891c8bd

Add before CCP1 and temporary loop opt bookmarks

047a7f9

Further explore IGV dump locations for loop opts

637f35d

Add enumeration support for IGV phase dumps. Add more loop opt dump p…

e60706d

…hases

Add a few more IGV dumps

60ce43e

Add remaining IGV dumps

8ab6949

Move phase iter counters to Compile instances

94749a7

Wrap new graph dumps in NOT_PRODUCT

9d64def

Change initialization for _igv_phase_iter

77be446

Address comments (partially)

1fd3fe3

Rename phases

9f1aeda

robcasloz reviewed Dec 7, 2023

View reviewed changes

src/hotspot/share/opto/compile.cpp Outdated Show resolved Hide resolved

Address comments

28c81d5

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 7, 2023

Remove leftover AFTER_SPLIT_IF

66f901a

dlunde requested review from robcasloz and chhagedorn December 7, 2023 10:03

dlunde added 2 commits December 7, 2023 12:25

Rename superword phases

ca180f0

Remove trailing whitespace

c2d001a

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 7, 2023

chhagedorn approved these changes Dec 7, 2023

View reviewed changes

src/hotspot/share/opto/loopopts.cpp Outdated Show resolved Hide resolved

src/hotspot/share/opto/phasetype.hpp Outdated Show resolved Hide resolved

test/hotspot/jtreg/compiler/lib/ir_framework/CompilePhase.java Outdated Show resolved Hide resolved

dlunde and others added 3 commits December 7, 2023 14:58

Update src/hotspot/share/opto/loopopts.cpp

9e71039

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

Update src/hotspot/share/opto/phasetype.hpp

c72efd1

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

Update test/hotspot/jtreg/compiler/lib/ir_framework/CompilePhase.java

6f872e5

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

chhagedorn approved these changes Dec 7, 2023

View reviewed changes

Update copyright

61be342

robcasloz approved these changes Dec 7, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 8, 2023

openjdk bot added the integrated Pull request has been integrated label Dec 8, 2023

openjdk bot closed this Dec 8, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Dec 8, 2023

dlunde deleted the 8295166-igv branch December 8, 2023 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8295166: IGV: dump graph at more locations #16120

8295166: IGV: dump graph at more locations #16120

dlunde commented Oct 10, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 10, 2023

openjdk bot commented Oct 10, 2023

TobiHartmann left a comment •

edited

Loading

TobiHartmann Oct 12, 2023

dlunde Oct 13, 2023

chhagedorn Nov 2, 2023 •

edited

Loading

dlunde Nov 6, 2023

chhagedorn Nov 6, 2023

dlunde Nov 6, 2023

dlunde commented Oct 13, 2023 •

edited

Loading

robcasloz left a comment

chhagedorn left a comment •

edited

Loading

chhagedorn Nov 2, 2023 •

edited

Loading

chhagedorn Nov 3, 2023

dlunde Nov 3, 2023

chhagedorn Nov 6, 2023

dlunde Nov 6, 2023

chhagedorn left a comment

robcasloz left a comment

dlunde commented Dec 8, 2023

openjdk bot commented Dec 8, 2023

robcasloz commented Dec 8, 2023

openjdk bot commented Dec 8, 2023

openjdk bot commented Dec 8, 2023

8295166: IGV: dump graph at more locations #16120

8295166: IGV: dump graph at more locations #16120

Conversation

dlunde commented Oct 10, 2023 • edited by openjdk bot Loading

Testing

Platforms: windows-x64, linux-x64, linux-aarch64, macosx-x64, macosx-aarch64

Platforms: linux-x64

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Oct 10, 2023

openjdk bot commented Oct 10, 2023

TobiHartmann left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chhagedorn Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlunde commented Oct 13, 2023 • edited Loading

robcasloz left a comment

Choose a reason for hiding this comment

chhagedorn left a comment • edited Loading

Choose a reason for hiding this comment

chhagedorn Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chhagedorn left a comment

Choose a reason for hiding this comment

robcasloz left a comment

Choose a reason for hiding this comment

dlunde commented Dec 8, 2023

openjdk bot commented Dec 8, 2023

robcasloz commented Dec 8, 2023

openjdk bot commented Dec 8, 2023

openjdk bot commented Dec 8, 2023

dlunde commented Oct 10, 2023 •

edited by openjdk bot

Loading

TobiHartmann left a comment •

edited

Loading

chhagedorn Nov 2, 2023 •

edited

Loading

dlunde commented Oct 13, 2023 •

edited

Loading

chhagedorn left a comment •

edited

Loading

chhagedorn Nov 2, 2023 •

edited

Loading