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

[Emanuel Peter]

On Fri, 22 Dec 2023 10:52:13 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 180:
> 
>> 178: //     Let node s be the next node being visited after node n in the DFS traversal. The following holds:
>> 179: //         n->in(i) = s
>> 180: class DFSNodeStack : public StackObj {
> 
> It could be nice to generalize this to a `DFSInputIterator`, which takes some filter function (either as lambda / functional or as a template argument for better inlining).
> I suspect we would use this in quite a few other places in the code, and this could reduce code duplication in the future, and make code much easier to read.

Hmm. You are doing more than a simple traversal though. so not sure how feasible this is.

> src/hotspot/share/opto/predicates.cpp line 237:
> 
>> 235: // Interface to transform OpaqueLoop* nodes of a Template Assertion Predicate Bool. The transformations must return a
>> 236: // new or different existing node.
>> 237: class TransformOpaqueLoopNodes : public StackObj {
> 
> Suggestion:
> 
> class TransformStrategyForOpaqueLoopNodes : public StackObj {
> 
> I would like to have the Strategy in it.
> And then you could rename:
> `CloneOpaqueLoopNodes` -> `CloneInitAndStride(Transform)StrategyForOpaqueLoopNodes`
> `CloneWithNewInit` -> `ReplaceInitAndCloneStride(Transform)StrategyForOpaqueLoopNodes`
> `ReplaceOpaqueLoopNodes` -> `ReplaceInitAndStride(Transform)StrategyForOpaqueLoopNodes`
> Yes, the names are longer, but it would be much clearer what they are for, in the places where they are used.
> 
> I was struggling to read `TemplateAssertionPredicateBool::clone`, and had to dig inside `clone_opaque_loop_nodes` to understand.

Also look at this comment: https://github.com/openjdk/jdk/pull/16877#discussion_r1434991996

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1434960576
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1434996269