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

[Emanuel Peter]

On Wed, 29 Nov 2023 08:42:41 GMT, Christian Hagedorn <chagedorn 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

Ok, this is it for now. I think it is awesome how you are refactoring the code, and packing it into classes to break up large methods 😊

src/hotspot/share/opto/predicates.cpp line 273:

> 271:     if (must_clone_node_on_top(transformed_opaque_loop_node)) {
> 272:       clone_and_replace_top_node();
> 273:     }

Why do you touch the output node here? Is it not post-visited later anyway?

src/hotspot/share/opto/predicates.cpp line 276:

> 274:     // Rewire the current node on top (child of old OpaqueLoop*Node) to the newly transformed node.
> 275:     rewire_node_on_top_to(transformed_opaque_loop_node);
> 276:   }

I would fuse these two methods:
`transform_opaque_loop_node` + `pop_transformed_opaque_loop_node`. That way you do not have to replace what is on the stack (which is nasty IMHO).

src/hotspot/share/opto/predicates.cpp line 286:

> 284:   // Predicate Bool.
> 285:   // If (1) is true then previously_visited_parent is part of the Template Assertion Predicate Bool. But if top was
> 286:   // already cloned, we do not need to clone it again to avoid duplicates.

I would make `top is not a clone` the first condition: obviously, if it is already cloned we do not need to clone again.
The other condition is a little worrying / more difficult to understand.
I think the idea is that `previously_visited_parent` would be the clone, if there was cloning on the current input.

src/hotspot/share/opto/predicates.cpp line 353:

> 351:   // Look for the OpaqueLoop* nodes to transform them with the strategy defined with 'transform_opaque_loop_nodes'.
> 352:   // Clone all nodes in between.
> 353:   BoolNode* clone(TransformOpaqueLoopNodes* transform_opaque_loop_nodes) {

This is a DFS. But how are the nodes cloned? In post-order, so after all inputs are processed and cloned, right?

src/hotspot/share/opto/predicates.cpp line 361:

> 359:         pop_transformed_opaque_loop_node();
> 360:       } else if (!_stack.push_next_unvisited_input()) {
> 361:         pop_node();

So this happens when no new input can be found, right? So a post-order traversal.

src/hotspot/share/opto/split_if.cpp line 98:

> 96:   }
> 97: 
> 98:   clone_template_assertion_predicate_bool_down_if_related(n);

We need a better name. I don't understand what it does from the name.

src/hotspot/share/opto/split_if.cpp line 413:

> 411: 
> 412: // This class clones Template Assertion Predicates Bools down as part of the Split If optimization.
> 413: class CloneTemplateAssertionPredicateBoolDown {

What does the `down` mean?

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16877#pullrequestreview-1794664577
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435190332
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435188973
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435198144
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435184937
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435185761
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435236754
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435234621