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

[Roland Westrelin]

On Tue, 7 May 2024 17:29:02 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> 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 don't think that's quite correct. Any round of igvn could cause the bounds of a counted loop to change in a way that conflicts with the types captured in the `CastII`/`ConvI2L` nodes. I think that's true even after loop optimizations are over. As a consequence, we want the Assertion Predicates to fold as late as possible. 

That's poorly tested currently because we emit the predicates in compiled code for debug builds so, in practice, we never really remove them.

As part of this change, I wouldn't change that behavior. That seems risky.

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

Fair enough.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18951#discussion_r1598493508
PR Review Comment: https://git.openjdk.org/jdk/pull/18951#discussion_r1598494163