RFR: 8325746: Refactor Loop Unswitching code

Emanuel Peter epeter at openjdk.org
Wed Feb 14 11:26:00 UTC 2024 

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

Nice Work @chhagedorn . I like that you packed things into classes. It nicely packs things into more digestable concepts. And allows you to create objects of those classes, and then pass then around as `const`, after which the reader knows they are not changed anymore. Very helpful!

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

> 198:   C->print_method(PHASE_BEFORE_LOOP_UNSWITCHING, 4, original_head);
> 199: 
> 200:   revert_to_normal_loop(original_head);

Can you add a comment that says why?

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

> 260:   if (loop_head->is_CountedLoop() && !loop_head->as_CountedLoop()->is_normal_loop()) {
> 261:     loop_head->as_CountedLoop()->set_normal_loop();
> 262:   }

Suggestion:

void PhaseIdealLoop::revert_to_normal_loop(const LoopNode* loop_head) {
  CountedLoopNode* cl = loop_head->isa_CountedLoop();
  if (cl != nullptr && !cl->is_normal_loop()) {
    cl->set_normal_loop();
  }

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

> 268:   LoopNode* const _strip_mined_loop_head;
> 269:   IdealLoopTree* const _loop;
> 270:   Node_List* const _old_new;

Could also be held "by reference". Then you don't have convert it from and to pointer. Well, idk, maybe it would be just as bad the other way around.

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

> 280:     Node* uniqc = proj_true->unique_ctrl_out();
> 281:     assert((uniqc == head && !head->is_strip_mined()) || (uniqc == head->in(LoopNode::EntryControl)
> 282:                                                           && head->is_strip_mined()), "must hold by construction if no predicates");

Is this code now simply unnecessary?

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

> 308:  public:
> 309:   // Unswitch on the invariant loop selector by creating two unswitched loop versions.
> 310:   void unswitch(const UnswitchedLoopSelector& unswitched_loop_selector) {

Just a suggestion, I am also ok with what it is now:
I wonder if the name `unswitch` is right here. Because at this point you only clone the loops, you don't yet remove the if. Maybe this should for now just be called "duplicate", and `create_unswitched_loop_versions` then really does the unswitching, by both duplicating and removing the ifs?

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

> 326: };
> 327: 
> 328: // Create a true-path- and a false-path-loop version of the original loop by cloning the loop and inserting an If to

Don't you already have the selector if at this point? The comment suggests you are inserting it here.

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

> 339: 
> 340: // Remove the unswitch candidate If nodes in both unswitched loop versions which are now dominated by the loop selector
> 341: // If node. Keep the true path block in the true-path-loop and the false path block in the false-path-loop by setting

As elsewhere: is it really right to talk about "blocks"? Or could the CFG be more complicated?

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

> 357:   IfNode* unswitch_candidate = unswitched_loop_selector.unswitch_candidate();
> 358:   IfNode* loop_selector = unswitched_loop_selector.selector();
> 359:   Node_List worklist;

Should there be a ResourceMark for this? There was none before either though.

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

> 365:       if (use->Opcode() == Op_CheckCastPP && loop->is_invariant(use->in(1))) {
> 366:         worklist.push(use);
> 367:       }

Suggestion:

      Node* check_cast = proj->fast_out(j)->isa_CheckCastPP();
      if (check_cast != nullptr && loop->is_invariant(check_cast->in(1))) {
        loop_invariant_check_casts.push(check_cast);
      }

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

> 369:     IfProjNode* loop_selector_if_proj = loop_selector->proj_out(proj->_con)->as_IfProj();
> 370:     while (worklist.size() > 0) {
> 371:       Node* cast = worklist.pop();

Suggestion:

    while (loop_invariant_check_casts.size() > 0) {
      CheckCastPPNode* cast = loop_invariant_check_casts.pop();

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

> 386:   for(int i = loop->_body.size() - 1; i >= 0 ; i--) {
> 387:     Node *n = loop->_body[i];
> 388:     Node *n_clone = old_new[n->_idx];

Suggestion:

    Node* n = loop->_body[i];
    Node* n_clone = old_new[n->_idx];

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17842#pullrequestreview-1879896486
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489240783
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489313234
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489308897
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489261684
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489304597
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489297245
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489269082
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489278480
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489282956
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489287409
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489289401

More information about the hotspot-compiler-dev mailing list