RFR: 8335257: Refactor code to create Initialized Assertion Predicates into separate class [v2]

[Emanuel Peter]

On Fri, 5 Jul 2024 08:56:48 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> This is the next patch for Assertion Predicates. It refactors the code to create an Initialized Assertion Predicate. Changes include:
>> 
>> - `clone_assertion_predicate_and_initialize()` currently does two things: Cloning a Template Assertion Predicate and creating an Initailized Assertion Predicate. I've split this method into two methods `clone_template_assertion_predicate()` and `create_initialized_assertion_predicate()`:
>>   - `clone_template_assertion_predicate()`: Now only clones the template. I have not cleaned the code up further because I will soon replace the `If` node with a dedicated `TemplateAssertionPredicateNode`. 
>>   - `create_initialized_assertion_predicate()`: I refactored the code for Initialized Assertion Predicate into a separate class `InitializedAssertionPredicate` which creates the complete Initialized Assertion Predicate `If` with a `HaltNode`. I could get rid of some of the arguments because they can be fetched inside the new class methods.
>>   - Moved `assertion_predicate_has_loop_opaque_node()` asserts to both methods.
>> - Renamed `AssertionPredicateType::Init_value` -> `AssertionPredicateType::InitValue` (same for last value).
>> 
>> Thanks,
>> Christian
>
> Christian Hagedorn has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - review Emanuel
>  - Merge branch 'refs/heads/master' into JDK-8335257
>  - 8335257: Refactor code to create Initialized Assertion Predicates into separate class

Nice, it looks really good now 😊

src/hotspot/share/opto/predicates.cpp line 405:

> 403: //                    Bool                                                new Bool
> 404: //                     |                                                     |
> 405: //                  Opaque4                ======>              control   OpaqueInitializedAssertionPredicate

Not sure if there is space, but it would be nice to name the `OpaqueInitializedAssertionPredicate` with `assertion_expression`

src/hotspot/share/opto/predicates.cpp line 427:

> 425:       assertion_expression.clone_and_replace_init_and_stride(_new_init, _new_stride, control, _phase);
> 426:   OpaqueInitializedAssertionPredicateNode* assertion_predicate_expression
> 427:       = new OpaqueInitializedAssertionPredicateNode(tmp_opaque->in(1)->as_Bool(), _phase->C);

Decide if you want the `=` on the first or last line and do it consistently ;)

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

Marked as reviewed by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19940#pullrequestreview-2160285851
PR Review Comment: https://git.openjdk.org/jdk/pull/19940#discussion_r1666542843
PR Review Comment: https://git.openjdk.org/jdk/pull/19940#discussion_r1666544274