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

[Roland Westrelin]

On Wed, 19 Jul 2023 07:12:05 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
>
> Christian Hagedorn has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Update src/hotspot/share/opto/predicates.hpp
>    
>    Co-authored-by: Tobias Hartmann <tobias.hartmann at oracle.com>
>  - Renaming Hoisted Predicate -> Hoisted Check Predicate in description and comments as discussed offline with Tobias, fixing additional typos in description

Otherwise, looks good to me.

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

> 322:   }
> 323:   register_control(new_iff, lp, entry);
> 324:   IfProjNode* if_cont;

Why is it ok to remove that code?

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

Marked as reviewed by roland (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14814#pullrequestreview-1549570415
PR Review Comment: https://git.openjdk.org/jdk/pull/14814#discussion_r1276108648