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

[Christian Hagedorn]

On Tue, 7 May 2024 15:40:40 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> 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?

That's correct. I've originally had these nodes as macro nodes as well. But concepttionally, we want to get these nodes to be removed and the Initialized Assertion Predicates folded once we know that we no longer split loops (i.e. in post loop IGVN). I think it's easier to register them for this post loop IGVN run since we don't really expand the nodes to anything - they are just removed during expansion.

I'm not entirely sure though what the original reason was to go with a macro expansion removal instead of a post loop IGVN removal for `Opaque4` nodes. Do you remember?

> 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?

I first thought about reusing this class in some way. But the second input is actually not needed. We could move forward and just remove the second input for `Opaque4` nodes (it's always a true constant). But I still wanted to have an easy way to have a distinguishable node from the other uses of the `Opaque4` nodes in non-null checks.

Furthermore, I think sub classing the `Opaque4` class can be problematic when doing `is_Opaque4()` since we sometimes expect an `Opaque4` only and sometimes an `OpaqueInitializedAssertionPredicate` only and sometimes both are fine. I think it's cleaner to have two separate classes instead of sub classing each other. 

What do you think?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18951#discussion_r1592838684
PR Review Comment: https://git.openjdk.org/jdk/pull/18951#discussion_r1592840333