RFR: 8330386: Replace Opaque4Node of Initialized Assertion Predicate with new OpaqueInitializedAssertionPredicateNode [v2]

[Roland Westrelin]

On Mon, 6 May 2024 14:24:27 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> This patch replaces the `Opaque4Node` of the `If` for Initialized Assertion Predicates with a new `OpaqueInitializedAsseritonPredicateNode`. This helps to simplify pattern matching for predicate code and to distinguish from the two other uses of `Opaque4` nodes:
>> 1. Template Assertion Predicate: The goal is to get rid of its `Opaque4Node` as well by using a dedicated `TemplateAssertionPredicateNode` for the `IfNode`.
>> 2. Non-null-checks with instrinsics and unsafe accesses: This will eventually be the only use left. Once we get there, we should rename the node accordingly to `OpaqueNonNullCheck` or something like that.
>> 
>> I went through all the uses of `Opaque4` nodes and did the following:
>> - Could the `Opaque4` node be part of an Initialized Assertion Predicate?
>>   - No: Added an assert that we are not dealing with an Initialized Assertion Predicate.
>>   - Yes:
>>     - Yes **and only** for Initialized Assertion Predicates? Added an assert that we are only expecting an `OpaqueInitializedAsseritonPredicateNode` if appropriate.
>>     - Yes but could also be something else: Added case for `OpaqueInitializedAsseritonPredicateNode` next to the `Opaque4` case.
>> - Is this `Opaque4` node only used for Template Assertion Predicates?
>>   - Yes: Added assert with call to `assertion_predicate_has_loop_opaque_node()` to check that we find its `OpaqueLoop*Nodes`.
>>  - I've added test cases where I was not sure about whether an `Opaque4` node could be part of a Template, an Initialized Assertion Predicate or a non-null-check. This was a little tricky but I think it was still worth to prevent future bugs (even though most of these special cases are quite rare).
>> 
>> This is another patch split off from the full fix for Assertion Predicates.
>> 
>> 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 four additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8330386
>  - Add more comments and asserts
>  - Add more tests
>  - 8330386: Replace Opaque4Node of Initialized Assertion Predicate with new OpaqueInitializedAssertionPredicateNode

src/hotspot/share/opto/opaquenode.cpp line 110:

> 108: }
> 109: 
> 110: Node* OpaqueInitializedAssertionPredicateNode::Identity(PhaseGVN* phase) {

Opaque4 is removed by macro expansion, right? But the new one is removed after loop opts.. So there's a change in behavior. What's the rationale for making that change?

src/hotspot/share/opto/opaquenode.hpp line 138:

> 136: // to true. Therefore, we get rid of them in product builds as they are useless. In debug builds we keep them as
> 137: // additional verification code (i.e. removing this node and use the BoolNode input instead).
> 138: class OpaqueInitializedAssertionPredicateNode : public Node {

Shouldn't the new OpaqueInitializedAssertionPredicateNode be a subclass of Opaque4 or shouldn't both be a subclass of a common super type? Don't they share at least some logic or behavior?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18951#discussion_r1592701811
PR Review Comment: https://git.openjdk.org/jdk/pull/18951#discussion_r1592702908