RFR: 8295166: IGV: dump graph at more locations
Christian Hagedorn
chagedorn at openjdk.org
Tue Dec 5 12:13:12 UTC 2023
On Tue, 10 Oct 2023 13:31:00 GMT, Daniel Lundén <duke at openjdk.org> wrote:
> This changeset
> 1. 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
> 2. adds support for enumeration of repeated IGV graph dumps.
> 3. 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](https://bugs.openjdk.org/browse/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.
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/compile.cpp line 5121:
> 5119: #endif
> 5120: }
> 5121:
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.
src/hotspot/share/opto/loopPredicate.cpp line 1276:
> 1274: offset, init, limit, stride, rng, overflow, reason);
> 1275:
> 1276: C->print_method(PHASE_AFTER_LOOP_PREDICATION_RC, 4, new_predicate_proj->in(0));
I thought about this here again. I propose to merge `PHASE_AFTER_LOOP_PREDICATION_RC` and `PHASE_AFTER_LOOP_PREDICATION_IC` to a single `PHASE_AFTER_LOOP_PREDICATION` phase and dump it after `dominated_by()` on L1289 which kills the hoisted check (replaces the bool with a constant). I think that's more intuitive. Otherwise, the old and the new `If` still share the same `BoolNode` in the dump. You can use `new_predicate_proj->in(0)` which is the same as `new_predicate_iff` for the invariant check.
src/hotspot/share/opto/loopPredicate.cpp line 1390:
> 1388: set_ctrl(zero, C->root());
> 1389:
> 1390: NOT_PRODUCT(C->print_method_iter(PHASE_BEFORE_LOOP_PREDICATION, 4, head);)
I suggest to move both the before and after phase into `PhaseIdealLoop::loop_predication_impl_helper()`, where we also do the dump with `TraceLoopOpts`. Additionally, we could also dump the node that's hoisted and the new predicate for it instead of the loop head. We could even define two separate phases for hoisting invariant checks and range checks.
It could look something like this if we try to hoist `20 IfNode` with predicate `30 IfNode`:
For invariant checks:
- `Before Loop Predication IC - IfNode 20`
- `After Loop Predication IC - IfNode 30`
For range checks:
- `Before Loop Predication RC - IfNode 20`
- `After Loop Predication RC - IfNode 30`
src/hotspot/share/opto/loopTransform.cpp line 802:
> 800: loop->record_for_igvn();
> 801:
> 802: C->print_method(PHASE_AFTER_LOOP_PEELING, 4, head);
You can use the new head after peeling here:
Suggestion:
C->print_method(PHASE_AFTER_LOOP_PEELING, 4, new_head);
src/hotspot/share/opto/loopTransform.cpp line 2390:
> 2388: #endif
> 2389:
> 2390: NOT_PRODUCT(C->print_method_iter(PHASE_UNROLL_LOOP, 4, loop_head);)
Here you could use the new loop head `clone_head` after unrolling
src/hotspot/share/opto/loopTransform.cpp line 2872:
> 2870: CountedLoopNode *cl = loop->_head->as_CountedLoop();
> 2871:
> 2872: NOT_PRODUCT(C->print_method_iter(PHASE_BEFORE_RANGE_CHECK_ELIMINATION, 4, cl);)
Here we could also try to dump the range check to be eliminated instead of the main loop head. There is no replacement though since we adjust the limits of the pre and main loop to eliminate this check. We could still think about dumping the main loop head in the `AFTER` phase as currently done.
src/hotspot/share/opto/loopUnswitch.cpp line 205:
> 203: #endif
> 204:
> 205: C->print_method(PHASE_AFTER_LOOP_UNSWITCHING, 4, head);
Here you can use the cloned slow loop head:
Suggestion:
C->print_method(PHASE_AFTER_LOOP_UNSWITCHING, 4, head_clone);
src/hotspot/share/opto/loopopts.cpp line 3902:
> 3900: #endif
> 3901:
> 3902: NOT_PRODUCT(C->print_method_iter(PHASE_PARTIAL_PEEL, 4, head);)
Here you can also dump the new head `new_head_clone` after partial peeling.
src/hotspot/share/opto/phasetype.hpp line 31:
> 29: flags(BEFORE_STRINGOPTS, "Before StringOpts") \
> 30: flags(AFTER_STRINGOPTS, "After StringOpts") \
> 31: flags(BEFORE_REMOVEUSELESS, "Before RemoveUseless") \
General comments here. I would add a `AFTER_` to match the `BEFORE_` phases for consistency where you also mention "After" in the name string.
src/hotspot/share/opto/phasetype.hpp line 36:
> 34: flags(ITER_GVN1, "Iter GVN 1") \
> 35: flags(AFTER_ITER_GVN_STEP, "After Iter GVN Step") \
> 36: flags(AFTER_ITER_GVN, "After Iter GVN") \
With the new `AFTER_ITER_GVN` phase that Roberto added some time ago, I think we can get rid of this one here together with Iter GVN 2.
src/hotspot/share/opto/phasetype.hpp line 38:
> 36: flags(AFTER_ITER_GVN, "After Iter GVN") \
> 37: flags(INCREMENTAL_INLINE_STEP, "Incremental Inline Step") \
> 38: flags(INCREMENTAL_INLINE_CLEANUP, "Incremental Inline Cleanup") \
We could use IGVN instead of Iter GVN which is more common to use. But I'm fine with both versions
src/hotspot/share/opto/phasetype.hpp line 50:
> 48: flags(BEFORE_BEAUTIFY_LOOPS, "Before beautify loops") \
> 49: flags(AFTER_BEAUTIFY_LOOPS, "After beautify loops") \
> 50: flags(BEFORE_UNROLL_LOOP, "Before loop unrolling") \
I suggest to use the same name as on the right side:
Suggestion:
flags(BEFORE_LOOP_UNROLLING, "Before loop unrolling") \
src/hotspot/share/opto/phasetype.hpp line 51:
> 49: flags(AFTER_BEAUTIFY_LOOPS, "After beautify loops") \
> 50: flags(BEFORE_LOOP_UNROLLING, "Before loop unrolling") \
> 51: flags(AFTER_LOOP_UNROLLING, "After loop unrolling") \
Nit: I suggest to use upper case letters for nouns in the new phase name strings to follow the convention of the other existing phases.
src/hotspot/share/opto/phasetype.hpp line 61:
> 59: flags(LOOP_PEEL, "After loop peeling") \
> 60: flags(BEFORE_LOOP_UNSWITCH, "Before loop unswitching") \
> 61: flags(LOOP_UNSWITCH, "After loop unswitching") \
Same here:
Suggestion:
flags(PARTIAL_PEELING, "After partial peeling") \
flags(BEFORE_LOOP_PEELING, "Before loop peeling") \
flags(LOOP_PEELING, "After loop peeling") \
flags(BEFORE_LOOP_UNSWITCHING, "Before loop unswitching") \
flags(LOOP_UNSWITCHING, "After loop unswitching") \
src/hotspot/share/opto/phasetype.hpp line 64:
> 62: flags(BEFORE_RANGE_CHECK_ELIMINATION, "Before range check elimination") \
> 63: flags(RANGE_CHECK_ELIMINATION, "After range check elimination") \
> 64: flags(BEFORE_PRE_POST_LOOPS, "Before pre/post loops") \
Here I suggest to use `BEFORE_PRE_MAIN_POST` to also mention the main loop which belongs to the pre and post loop. Same for the string on the right side.
src/hotspot/share/opto/phasetype.hpp line 76:
> 74: flags(PHASEIDEALLOOP1, "PhaseIdealLoop 1") \
> 75: flags(PHASEIDEALLOOP2, "PhaseIdealLoop 2") \
> 76: flags(PHASEIDEALLOOP3, "PhaseIdealLoop 3") \
I guess we can remove that as well since we already have `AFTER_EA` and `AFTER_ITER_GVN`.
src/hotspot/share/opto/phasetype.hpp line 77:
> 75: flags(PHASEIDEALLOOP2, "PhaseIdealLoop 2") \
> 76: flags(PHASEIDEALLOOP3, "PhaseIdealLoop 3") \
> 77: flags(BEFORE_CCP1, "Before PhaseCCP 1") \
I suggest to rename this to `AFTER_MACRO_NODE_ELIMINATION` and move it just before `igvn.optimize()` in the code. Otherwise, this phase is a duplication of `AFTER_ITER_GVN`.
src/hotspot/share/opto/phasetype.hpp line 80:
> 78: flags(CCP1, "PhaseCCP 1") \
> 79: flags(ITER_GVN2, "Iter GVN 2") \
> 80: flags(PHASEIDEALLOOP_ITERATIONS, "PhaseIdealLoop iterations") \
With the new counters, we could now specify a single `PHASE_IDEAL_LOOP` and use that one for these phases and for `PHASEIDEALLOOP_ITERATIONS`.
src/hotspot/share/opto/phasetype.hpp line 82:
> 80: flags(PHASEIDEALLOOP_ITERATIONS, "PhaseIdealLoop iterations") \
> 81: flags(MACRO_EXPANSION, "Macro expand") \
> 82: flags(BARRIER_EXPANSION, "Barrier expand") \
Since there is only one run of CCP, I suggest to remove "1" and also "phase".
Suggestion:
flags(BEFORE_CCP, "Before CCP") \
flags(CCP, "After CCP") \
src/hotspot/share/opto/superword.cpp line 2410:
> 2408: #endif
> 2409:
> 2410: CountedLoopNode *cl = lpt()->_head->as_CountedLoop();
Asterisk should be at type:
Suggestion:
CountedLoopNode* cl = lpt()->_head->as_CountedLoop();
-------------
PR Review: https://git.openjdk.org/jdk/pull/16120#pullrequestreview-1710654550
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1381622519
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1386246842
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1380370224
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1386106559
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1381643702
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1381648394
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1386122306
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1381653756
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1380387096
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1381606655
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1381607678
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1381594341
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1386263247
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1381597191
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1381599029
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1381610367
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1381612937
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1381602356
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1380391478
PR Review Comment: https://git.openjdk.org/jdk/pull/16120#discussion_r1386124951
More information about the hotspot-compiler-dev
mailing list