RFR: 8305638: Refactor Template Assertion Predicate Bool creation and Predicate code in Split If and Loop Unswitching

Fri Dec 22 15:48:10 UTC 2023

On Fri, 22 Dec 2023 13:10:56 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> This patch is intended for JDK 23.
>> 
>> While preparing the patch for the full fix for Assertion Predicates [JDK-8288981](https://bugs.openjdk.org/browse/JDK-8288981), I still noticed that some changes are not required for the actual fix and could be split off and reviewed separately in this PR.
>> 
>> The patch applies the following cleanup changes:
>> - The complete fix had to add slightly different cloning cases in `PhaseIdealLoop::create_bool_from_template_assertion_predicate()` which already has quite some logic to switch between different cases. Additionally, the algorithm in the method itself was already hard to understand and difficult to adapt. I therefore re-implemented it in a separate class `CloneTemplateAssertionPredicateBool` together with some helper classes like `DFSNodeStack`. To use it, I've added a `TemplateAssertionPredicateBool` class that offers three cloning possibilities:
>>   - `clone()`: Clone without modification
>>   -  `clone_and_replace_opaque_loop_nodes()`: Clone and replace the `OpaqueLoop*Nodes` with a new init and stride node.
>>   - `clone_and_replace_init()`: Special case of `clone_and_replace_opaque_loop_nodes()` which only replaces `OpaqueLoopInitNode` and clones `OpaqueLoopStrideNode`.
>>   
>>   This refactoring could be extracted from the complete fix.
>> - The Split If code to detect (`subgraph_has_opaque()`) and clone Template Assertion Predicate Bools was extracted to a separate class `CloneTemplateAssertionPredicateBoolDown` and uses the new `TemplateAssertionPredicateBool` class to do the actual cloning.
>> - In the process of coding the complete fix, I've refactored the Loop Unswitching code quite a bit. This change could also be extracted into a separate RFE. Changes include:
>>   - Renaming
>>   - Extracting code to separate classes/methods
>>   - Adding comments
>> - Some small refactoring including:
>>   - Removing unused parameters
>>   - Renaming variables/parameters/methods
>> 
>> Thanks,
>> Christian
>
> src/hotspot/share/opto/loopUnswitch.cpp line 117:
> 
>> 115: 
>> 116: // Perform Loop Unswitching on the loop containing an invariant test that does not exit the loop. The loop is cloned
>> 117: // such that we have two identical loops next to each other - a fast and a slow loop. We modify the loops as follows:
> 
> It sounds like they are somehow identical and at the same time modified.
> Suggestion: say that they are "first" cloned to be identical, and then "second" modified such that one becomes the "true-path" and the other the "false-path" loop. I wonder if we can eliminate the "fast / slow" wording, because who really knows which path is faster or slower, right?

Also: why can the invariant if not be eliminated in the loops as a dominating if, with and independent optimization? Would that not be cleaner? Well I guess maybe we just want to be sure that it happens.

> src/hotspot/share/opto/loopUnswitch.cpp line 266:
> 
>> 264:         _selector(create_selector_if(loop, unswitch_if_candidate)),
>> 265:         _fast_loop_proj(create_fast_loop_proj()),
>> 266:         _slow_loop_proj(create_slow_loop_proj()) {}
> 
> It seems to me you could make all fields `const` of some sort, right?

You will never modify the pointers again.

> src/hotspot/share/opto/loopUnswitch.cpp line 300:
> 
>> 298:   void fix_loop_entries(IfProjNode* iffast_pred, IfProjNode* ifslow_pred) {
>> 299:     _phase->replace_loop_entry(_strip_mined_loop_head, iffast_pred);
>> 300:     LoopNode* slow_loop_strip_mined_head = _old_new->at(_strip_mined_loop_head->_idx)->as_Loop();
> 
> utility method `old_to_new` could be helpful, and eliminate this line.

It would also make the other code below easier to read, I think

> src/hotspot/share/opto/loopUnswitch.cpp line 325:
> 
>> 323:         _loop(loop),
>> 324:         _old_new(old_new),
>> 325:         _phase(loop->_phase) {}
> 
> Again, I think it would be nice to have the constructor and public member methods at the beginning, right after the field definition. It just helps to see how they belong together better.

And the fields could be const.

> src/hotspot/share/opto/loopUnswitch.cpp line 334:
> 
>> 332:     const uint first_slow_loop_node_index = _phase->C->unique();
>> 333:     _phase->clone_loop(_loop, *_old_new, _phase->dom_depth(_loop_head),
>> 334:                        PhaseIdealLoop::CloneIncludesStripMined, loop_selector);
> 
> I think this used to be:
> `clone_loop(loop, old_new, dom_depth(head->skip_strip_mined()), mode, iff);`
> Should it be `dom_depth` of `_strip_mined_loop_head`?

Also there used to be an assert in the old code after the clone: `assert(old_new[head->_idx]->is_Loop(), "" );`
not sure if worth keeping?

> src/hotspot/share/opto/predicates.cpp line 212:
> 
>> 210:   }
>> 211: 
>> 212:   uint node_index_to_previously_visited_parent() const {
> 
> This is the "parent" of the current node, right?
> Parent is also a bit confusing, I think. Maybe use "input" instead? Because at the beginning you say the DFS traverses the inputs.
> Why do you say "previously visited"?

Ah. I think it should simply be `top_input_index`. You could then also rename `top` -> `top_node`. And `increment_top_node_input_index` -> `increment_top_index`, if you even think it is worth keeping.

> src/hotspot/share/opto/predicates.hpp line 270:
> 
>> 268: // A Template Assertion Predicate Bool represents the BoolNode for the initial value or the last value of a
>> 269: // Template Assertion Predicate and all the nodes up to and including the OpaqueLoop* nodes.
>> 270: class TemplateAssertionPredicateBool : public StackObj {
> 
> The current name targets the "bool" node, and it is not clear that you actually mean to represent the whole assertion-predicate expression. Hence:
> `TemplateAssertionPredicateBool` -> `TemplateAssertionPredicateExpression`
> 
> Then `could_be_part` makes more sense. Maybe still, you could rename it to `maybe_contains`?
> But is it really only "maybe" or "could"? Maybe add some explanation in what cases it looks like it could be part of it, but is actually not?
> 
> Also, expression would help to make more sense of clone, since it clones the whole expression!

And why not also include the `Opaque4` node below the bool node?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435045548
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435090723
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435136176
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435131946
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435138897
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435165087
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435155525