RFR: 8295166: IGV: dump graph at more locations

Tue Dec 5 12:13:19 UTC 2023

On Fri, 3 Nov 2023 12:39:57 GMT, Christian Hagedorn <chagedorn 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)
>> ![Screenshot from 2023-12-04 13-55-38](https://github.com/openjdk/jdk/assets/4222397/6759dc5a-9c9a-42b9-8d9e-2d0b53e76ab4) ![Screenshot from 2023-12-04 13-56-29](https://github.com/openjdk/jdk/assets/4222397/44d6a239-587b-4f7c-8ce1-f7613cb2fa35)
>> 
>> 
>> 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.
>
> 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.

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?

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

Good, now fixed. Just to be clear, I merged the two `AFTER` phases but did not touch the `BEFORE` phases.

> 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`

Updated now

> 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

Updated

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

Updated now

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

Thanks, updated.

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

I believe I've kept it consistent for my own additions, but the older phase names are sometimes inconsistent in this regard (including `STRINGOPTS`). Should I rename other phases to improve consistency? This changeset will touch many more files then, but perhaps that's OK.

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