RFR: 8325746: Refactor Loop Unswitching code [v4]

[Emanuel Peter]

On Wed, 14 Feb 2024 14:25:31 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).
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Change block -> path

Thanks for the updates, we are almost there :)

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

> 42: // - Cloned loop -> False-path-loop:  The false-path-path of the invariant, non-loop-exiting test in the cloned loop
> 43: //                                    is kept while the true-path-path is killed. We call this unswitched loop version
> 44: //                                    the false-path loop.

Suggestion:

// - Original loop -> true-path-loop: The true-path of the invariant, non-loop-exiting test in the original loop
//                                    is kept while the false-path is killed. We call this unswitched loop version
//                                    the true-path-loop.
// - Cloned loop -> False-path-loop:  The false-path of the invariant, non-loop-exiting test in the cloned loop
//                                    is kept while the true-path is killed. We call this unswitched loop version
//                                    the false-path loop.

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

> 68: //    Endloop                                                   cloned if-path                    [cloned else-path]
> 69: //                                                              cloned stmt2                      cloned stmt2
> 70: //                                                            Endloop                           Endloop

Could it be the other way around: the if-path is empty and the else-path has code? Or are you just trying to suggest that there could be an empty path, and that this would really simplify one of the cloned loops now?

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

> 205: // becomes the false-path-loop while original loop becomes the true-path-loop.
> 206: class OriginalLoop : public StackObj {
> 207:   LoopNode* const _strip_mined_loop_head;

Suggestion:

  LoopNode* const _loop_head; // OuterStripMinedLoopNode if loop strip mined, else just the loop head.

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

> 212:  public:
> 213:   OriginalLoop(IdealLoopTree* loop, Node_List& old_new)
> 214:       : _strip_mined_loop_head(loop->_head->as_Loop()->skip_strip_mined()),

Hmm. this could now be a strip mined loop node, or a regular loop node (if loop not strip mined), right?
Maybe you could just call it `_loop_head`, and leave a comment, see suggestion above.

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17842#pullrequestreview-1880425858
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489552746
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489555485
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489569675
PR Review Comment: https://git.openjdk.org/jdk/pull/17842#discussion_r1489565373