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

[Christian Hagedorn]

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

src/hotspot/share/opto/loopPredicate.cpp line 108:

> 106:   ParsePredicateNode* parse_predicate = parse_predicate_success_proj->in(0)->as_ParsePredicate();
> 107:   ParsePredicateUncommonProj* uncommon_proj = parse_predicate->uncommon_proj();
> 108:   Node* uncommon_trap = parse_predicate->uncommon_trap();

Apart from variable renaming in this method, I've changed the code to use `uncommon_proj()` and `uncommon_trap()` instead.

src/hotspot/share/opto/loopPredicate.cpp line 789:

> 787: //   max(scale*i + offset) = scale*init + offset
> 788: BoolNode* PhaseIdealLoop::rc_predicate(Node* ctrl, const int scale, Node* offset, Node* init, Node* limit,
> 789:                                        const jint stride, Node* range, const bool upper, bool& overflow) {

`loop` was unused and is now removed.

src/hotspot/share/opto/loopPredicate.cpp line 1273:

> 1271:     if (TraceLoopPredicate) {
> 1272:       tty->print_cr("upper bound check if: %d", upper_bound_iff->_idx);
> 1273:     }

@dlunde noticed this copy past bug of using `lower_bound_iff` instead of `upper_boundd_iff`. Fixed that here as well

src/hotspot/share/opto/loopPredicate.cpp line 1356:

> 1354:     return false;
> 1355:   }
> 1356: 

Now covered by `can_apply_loop_predication()`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1408940678
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1408938198
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1408939176
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1408939610