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

[Emanuel Peter]

On Fri, 22 Dec 2023 15:36:18 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/predicates.cpp line 245:
> 
>> 243: // Class to clone a Template Assertion Predicate Bool. The BoolNode and all the nodes up to but excluding the OpaqueLoop*
>> 244: // nodes are cloned. The OpaqueLoop* nodes are transformed by the provided strategy (e.g. cloned or replaced).
>> 245: class CloneTemplateAssertionPredicateBool : public StackObj {
> 
> Suggestion:
> 
> class CloneTemplateAssertionPredicateExpression : public StackObj {

I left quite a few comments below, because I think the algorithm is difficult to understand.
I like the idea of splitting it into smaller parts.

If I understand the basic algorithm, we do this:
DFS, where we traverse the inputs recursively, using the `DFSNodeStack`, which uses the filter `TemplateAssertionPredicateBool::could_be_part`.

If you see a init/stride `Opaque1` node, then you obviously clone it.

If you come back from an input to a output (use to def), then you may get back a cloned input node.
If the input node is not a clone, then we have to do nothing.
If the input node is a clone, we also have to clone the output node. Clone it if it is not already a clone. And then update the current input slot, so we do not point to the pre-cloned input, but to the cloned input.

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

Oh, I see. We are eagerly cloning the output node, if it is not yet cloned. Hmm. I guess that is better than trying to fiture out in the post-visit if we need to clone or not, because that would require querying if any input node is a clone?

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

I think it would be better to have two methods:
`post_visit_opaque_loop_node`
`post_visit_other_node`
And then call pop explicitly here. That way, we only push and pop here, and the traversal is a bit easier to understand.

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

I have not yet reviewed this part of the code. I'm out of time for today / this year ;)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435214644
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435192504
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435188044
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435237075