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

[Christian Hagedorn]

On Mon, 13 May 2024 13:37:24 GMT, Roland Westrelin <roland at openjdk.org> wrote:

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

I see your point and agree with it. While Template Assertion Predicates should be removed after loop opts are over (no more splitting possible and thus no creation of new Initialized Assertion Predicates) we should indeed delay the removal of the Initialized Assertion Predicates to keep the graph sane. 

Ideally, they should be removed in the very last IGVN round. But there is currently no such dedicated "last IGVN round" which we could register a node for. Doing the removal in macro expansion is probably fine and as you've stated, it does not change the current behavior.

I've pushed an update doing the change back to macro node.

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

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