RFR: 8325746: Refactor Loop Unswitching code [v3]

[Christian Hagedorn]

On Wed, 14 Feb 2024 09:52:15 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Suggestions by Emanuel
>
> src/hotspot/share/opto/loopUnswitch.cpp line 41:
> 
>> 39: //                                                                    [Initialized Assertion Predicates]
>> 40: //                                                                                    |
>> 41: //    [Predicates]                                                             if (invariant-test)
> 
> Suggestion:
> 
> //    [Predicates]                                                             if (invariant-test turned into loop selector)

Changed it into `loop selector If (invariant-test)`

> src/hotspot/share/opto/loopUnswitch.cpp line 130:
> 
>> 128:   UnswitchedLoopSelector(IdealLoopTree* loop)
>> 129:       : _phase(loop->_phase),
>> 130:         _outer_loop(loop->_head->as_Loop()->is_strip_mined() ? loop->_parent->_parent : loop->_parent),
> 
> Is there no utility method for this?
> Hmm, maybe not on `IdealLoopTree` side.
> 
> But I do find other code like this:
> `head->is_strip_mined() ? loop->_parent : loop`
> 
> Maybe you could make a utility method then: `IdealLoopTree::skip_strip_mined()`, just like the one for `LoopNode`.

Added and replaced the other usage with it as well.

> src/hotspot/share/opto/loopUnswitch.cpp line 164:
> 
>> 162:     } else {
>> 163:       proj_to_loop = new IfFalseNode(_selector);
>> 164:     }
> 
> Suggestion:
> 
>     IfProjNode* proj_to_loop = (path_to_loop == TRUE_PATH) ? new IfTrueNode(_selector)
>                                                              new IfFalseNode(_selector);
> 
> A question of taste. Just an idea.

Needed to revert this back due to:

error: conditional expression between distinct pointer types 'IfTrueNode*' and 'IfFalseNode*' lacks a cast
  162 |     proj_to_loop = path_to_loop == TRUE_PATH ? new IfTrueNode(_selector) : new IfFalseNode(_selector);

I think then it's cleaner to use the explicit `if-else`.

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

Added a comment at the method itself.

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

Done

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

I've moved `remove_unswitch_candidate_from_loop()` into this method to really have the complete unswitching process in this method. This also allowed to get rid of  `create_unswitched_loop_versions()` which now was only a redundant delegation. This required to move the `OriginalLoop` class further up.

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

Fixed in the updated comment at `OriginalLoop::unswitch()`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489360502
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489486865
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489412841
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489358152
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489490248
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489506956
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489507626