RFR: 8325746: Refactor Loop Unswitching code

[Christian Hagedorn]

On Wed, 14 Feb 2024 09:06:03 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> To simplify the review of https://github.com/openjdk/jdk/pull/16877, I've decided to split off some code into separate sub-tasks.
> 
> This sub-task focuses on the code refactoring done in Loop Unswitching. I've incorporated the feedback from @eme64 at https://github.com/openjdk/jdk/pull/16877 in the proposed changes.
> 
> #### Changes
> - Adding a more detailed description about the Loop Unswitching optimization itself below the file header.
> - Renaming `fast loop` -> `true-path-loop` and `slow loop` -> `false-path-loop` since we don't really know which one is faster and to avoid keeping a mental map of which path goes into which unswitched loop version.
> - Creating new classes:
>   - `UnswitchedLoopSelector` to create the unswitched loop selector `If` (i.e. the If node that selects at runtime which unswitched loop version is executed) based on the invariant and non-loop-exiting unswitch candidate `If`.
>   - `OriginalLoop` to do the actual unswitching (i.e. loop cloning, predicates cloning and fixing the graph to the loop selector `If`).
> - Extracting methods in `do_unswitching()` to clean it up further.
> - Improving `TraceLoopUnswitching` result printing.
> - Adding some shared utility methods.
> 
> #### Possible Follow-Up RFEs
> There are possible follow-up opportunities to further improve the loop unswitching which, however, would go to far in the context of fixing Assertion Predicates. We could follow up with RFEs to tackle these:
> - At some point it might be good to have a dedicated `LoopUnswitching` class to split all the methods off from `PhaseIdealLoop`. This would also avoid noisy "loop_unswitching" prefixes.
> - Bailout code in `has_control_dependencies_from_predicates()` is too conservative. I have already some code to improve that in the full fix for Assertion Predicates which I plan to split off and propose separatly.
> - `TraceLoopUnswitching` currently only prints the result and if it failed. We might want to improve that in the future to cover more steps to make it useful for debugging (would require some more fine-grained debug levels, though).

src/hotspot/share/opto/loopUnswitch.cpp line 133:

> 131:   if (TraceLoopOpts) {
> 132:     tty->print("Unswitch   %d ", head->unswitch_count()+1);
> 133:     loop->dump_head();

Extracted to `trace_loop_unswitching_count()`

src/hotspot/share/opto/loopUnswitch.cpp line 141:

> 139:   // Need to revert back to normal loop
> 140:   if (head->is_CountedLoop() && !head->as_CountedLoop()->is_normal_loop()) {
> 141:     head->as_CountedLoop()->set_normal_loop();

Extracted to `revert_to_normal_loop()`

src/hotspot/share/opto/loopUnswitch.cpp line 146:

> 144:   IfNode* invar_iff = create_slow_version_of_loop(loop, old_new, unswitch_iff, CloneIncludesStripMined);
> 145:   ProjNode* proj_true = invar_iff->proj_out(1);
> 146:   verify_fast_loop(head, proj_true);

Refactored to `create_unswitched_loop_versions()`.

src/hotspot/share/opto/loopUnswitch.cpp line 152:

> 150:   int nct = head->unswitch_count() + 1;
> 151:   head->set_unswitch_count(nct);
> 152:   head_clone->set_unswitch_count(nct);

Extracted to `increment_unswitch_counts()`.

src/hotspot/share/opto/loopUnswitch.cpp line 187:

> 185:   _igvn.rehash_node_delayed(unswitch_iff_clone);
> 186:   ProjNode* proj_false = invar_iff->proj_out(0);
> 187:   dominated_by(proj_false->as_IfProj(), unswitch_iff_clone);

Extracted to `remove_unswitch_candidate_from_loops()` and put inside `create_unswitched_loop_versions()`.

src/hotspot/share/opto/loopUnswitch.cpp line 195:

> 193:     NOT_PRODUCT(trace_loop_unswitching_impossible(original_head);)
> 194:     return;
> 195:   }

Extracted to `hoist_invariant_check_casts()`

src/hotspot/share/opto/loopUnswitch.cpp line 195:

> 193:     Node *n_clone = old_new[n->_idx];
> 194:     _igvn._worklist.push(n_clone);
> 195:   }

Extracted to `add_unswitched_loop_version_bodies_to_igvn()`

src/hotspot/share/opto/loopUnswitch.cpp line 203:

> 201:                   old_new[head->_idx]->_idx, unswitch_iff_clone->_idx);
> 202:   }
> 203: #endif

Extracted to `trace_loop_unswitching_result()`

src/hotspot/share/opto/loopopts.cpp line 2871:

> 2869:   assert(opcode == Op_If || opcode == Op_RangeCheck, "unexpected opcode");
> 2870:   IfNode* new_if = (opcode == Op_If) ? new IfNode(proj2, bol, iff->_prob, iff->_fcnt):
> 2871:     new RangeCheckNode(proj2, bol, iff->_prob, iff->_fcnt);

Since I re-used this pattern again, I extracted this into `make_with_same_profile()`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489140726
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489141201
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489144869
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489142232
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489145537
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489141757
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489142987
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489143360
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489151022