RFR: 8305636: Expand and clean up predicate classes and move them into separate files

[Tobias Hartmann]

On Mon, 10 Jul 2023 15:17:37 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> This is the third clean-up PR towards fixing issues with Assertion Predicates ([JDK-8288981](https://bugs.openjdk.org/browse/JDK-8288981)). This patch does not change anything in the way the old Assertion Predicates work.
> 
> After collecting and moving the predicate code in the last clean-up PR https://github.com/openjdk/jdk/pull/14017 to the classes `Predicates/ParsePredicates`, I'm now completely moving the code to separate `predicates.cpp/hpp` files. By doing so, I also updated the predicate description and updated some namings. Since this description is also moved to the new files, I've committed the description update separately to better reflect these changes.
> 
> Changes include:
> - Moved `Predicates/ParsePredicates` classes to new files `predicates.cpp/hpp`.
> - Turning the `Predicates` utility class into a real class to represent all predicates:
>   - Contains three `PredicateBlock` fields for each Predicate Block (see description of `Predicate Block`).
>     - The `PredicateBlock` class offers methods to query the presence of predicates and to access them (e.g. get the Parse Predicate projection).
>   - In the process, the `ParsePredicates` could be removed as the Parse Predicates are now covered by the `PredicateBlock` class.
> - New `AssertionPredicatesWithHalt` class to skip over Assertion Predicates (will be further cleaned up later with the complete fix in JDK-8288981).
> - Updated predicate description and moved to `predicates.hpp`.
>   - While testing a prototype fix of JDK-8288981, I've came to the conclusion that we should not move all Assertion Predicates to a separate block below the Parse and Hoisted Predicates, because it prevented further application of Loop Predication due to pins of data nodes to these Assertion Predicates while the Hoisted Predicates needed them above the Assertion Predicates (i.e. dominance problems leading to bad graph assertions). I've removed that part of the description that gave a heads-up about that change.
> - Small clean-ups such as variable renaming or code move.
> 
> Not included:
> - Refactoring predicate traversal to clone/copy/initialize predicates for loop unswitching, pre/main/post, loop peeling etc. (this is only done in the actual fix in JDK-8288981 which requires some updates anyways - so this refactoring is not done here (yet)).
> 
> Testing: Tier1-7, hs-precheckin-comp, hs-comp-stress
> 
> Thanks,
> Christian

Nice refactoring. Looks good to me otherwise!

src/hotspot/share/opto/predicates.hpp line 52:

> 50:  *                                              "a[i*scale + offset]", where scale and offset are loop-invariant, out of
> 51:  *                                              a counted loop. Each Hoisted Range Check Predicate is accompanied by
> 52:  *                                              additional Assertion Predicates.

As we discussed offline, I would suggest to remove this part.

src/hotspot/share/opto/predicates.hpp line 54:

> 52:  *                                              additional Assertion Predicates.
> 53:  *         - Loop Predicate:     This Hoisted Predicate is created to hoist a loop-invariant check a range check of the
> 54:  *                               form "a[i*scale + offset]", where scale and offset are loop-invariant, out of a

This needs re-phrasing: "a loop-invariant check a range check"

src/hotspot/share/opto/predicates.hpp line 72:

> 70:  * - Assertion Predicate: An always true predicate which will never fail (its range is already covered by an earlier
> 71:  *                        Hoisted Predicate or the main-loop entry guard) but is required in order to fold away a dead
> 72:  *                        sub loop inside which some data could be proven to be dead (by the type system) and replaced

This needs re-phrasing: "a dead sub loop inside which some data could be"

src/hotspot/share/opto/predicates.hpp line 136:

> 134:  *                                                   other iterations of the main-loop in-between by implication.
> 135:  *                                                   Note that Range Check Elimination could remove additional range
> 136:  *                                                   checks which we were not possible to remove with Loop Predication

Suggestion:

 *                                                   checks which were not possible to remove with Loop Predication

src/hotspot/share/opto/predicates.hpp line 211:

> 209:   AssertionPredicatesWithHalt(Node* assertion_predicate_proj) : _entry(find_entry(assertion_predicate_proj)) {}
> 210: 
> 211:   // Returns the control input node into the first assertion predicate If. If there are no assertion predicates, it.

Suggestion:

  // Returns the control input node into the first assertion predicate If. If there are no assertion predicates, it

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14814#pullrequestreview-1536307400
PR Review Comment: https://git.openjdk.org/jdk/pull/14814#discussion_r1267597982
PR Review Comment: https://git.openjdk.org/jdk/pull/14814#discussion_r1267573771
PR Review Comment: https://git.openjdk.org/jdk/pull/14814#discussion_r1267578402
PR Review Comment: https://git.openjdk.org/jdk/pull/14814#discussion_r1267602419
PR Review Comment: https://git.openjdk.org/jdk/pull/14814#discussion_r1267605184