RFR: 8342043: Split Opaque4Node into OpaqueTemplateAssertionPredicateNode and OpaqueNotNullNode

[Christian Hagedorn]

On Mon, 21 Oct 2024 12:46:16 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> ### Two Uses of `Opaque4`
> The `Opaque4` node is currently used for two things:
> 1. Non-null checks of intrinsics to accompany a cast to non-null where we implicitly know that an object is non-null but C2 does not. This is required to fold control away if the cast becomes top at some point.
> 2. Template Assertion Predicates
> 
> ### How to Differentiate between Uses
> The only reliable way to differentiate between the two uses is to check if there are `OpaqueLoop*Nodes` above the `Opaque4` node which should only be found for a Template Assertion Predicate. This can be done with the method `assertion_predicate_has_loop_opaque_node()` which follows the inputs of the `Opaque4` recursively.
> 
> ### Problems by Sharing `Opaque4` Nodes for Two Concepts
> This sharing of the `Opaque4` comes with some problems:
> - One need to be careful when checking for `Opaque4` nodes if the code can be applied for non-null checks and/or Template Assertion Predicates. Ideally, we should add assertions to exclude the unexpected case (mostly done).
> - Walking the graph with `assertion_predicate_has_loop_opaque_node()` has some overhead, especially when checking the negation, we could walk quite some nodes in the worst case.
> - There is no real benefit of (re)using `Opaque4` nodes for two concepts and could easily be separated.
> 
> ### Split `Opaque4` into Two Classes to Separate Uses
> Therefore, this patch proposes to split `Opaque4` into a new `OpaqueNonNull` and a `OpaqueTemplateAssertionPredicate` class (the latter accompanies the already split off `OpaqueInitializedAssertionPredicate` class). I went through all the uses and updated them accordingly.
> 
> As a consequence, I could turn `assertion_predicate_has_loop_opaque_node()` into a debug only method since it's now only used in assertion code. I additionally removed the second input of the `Opaque4` nodes since these nodes have always eventually been replaced by true.
> 
> ### Turning `OpaqueTemplateAssertionPredicate` into a Non-Macro Node
> The `Opaque4` was a macro node. The `OpaqueNotNull` still is but the `OpaqueTemplateAssertionPredicate` is turned into a non-macro node and is removed after loop opts. At  that point, loops can no longer be split and we do no need to create Initialized Assertion Predicates from the templates anymore. Thus, they can be cleaned up in the post loop opts IGVN.
> 
> ### Pre-Requisite for Replacing Uncommon Traps with Halt Nodes for Template Assertion Predicates
> Eventually, I want to get rid of UCTs for Template Assertion Pre...

src/hotspot/share/opto/escape.cpp line 584:

> 582:             can_reduce = (opc == Op_CmpP || opc == Op_CmpN) && can_reduce_cmp(n, iff_cmp);
> 583:           } else {
> 584:             assert(iff->in(1)->is_OpaqueNotNull(), "must be OpaqueNotNull");

I don't think we can have a Template Assertion Predicate here, so I added an assertion. If this ever fails, we can just remove the assertion add a test for it.

src/hotspot/share/opto/loopTransform.cpp line 1195:

> 1193:                bol->is_OpaqueTemplateAssertionPredicate() ||
> 1194:                bol->is_OpaqueInitializedAssertionPredicate(),
> 1195:                "Opaque node of a non-null-check or an Assertion Predicate");

Need case for both new opaque nodes (hit during testing).

src/hotspot/share/opto/loopopts.cpp line 2205:

> 2203:       // split if to break.
> 2204:       assert(!use->is_OpaqueTemplateAssertionPredicate(),
> 2205:              "should not clone a Template Assertion Predicate which should be removed once it's useless");

`OpaqueTemplateAssertionPredicate` nodes should only have a single unique use. I therefore added this assert here.

src/hotspot/share/opto/macro.cpp line 2431:

> 2429:       default:
> 2430:         assert(n->Opcode() == Op_LoopLimit ||
> 2431:                n->is_OpaqueNotNull()       ||

`OpaqueTemplateAssertionPredicate` is not a macro node.

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

> 144:   }
> 145:   IfNode* if_node = node->in(0)->as_If();
> 146:   return if_node->in(1)->is_OpaqueTemplateAssertionPredicate();

Detection is now easier. I plan to add more verification code that we also find the correct nodes on the failing path. But I'll wait with that until we have only Halt nodes for Template Assertion Predicates.

src/hotspot/share/opto/split_if.cpp line 326:

> 324:             Node* use = bol->unique_out();
> 325:             if (use->is_OpaqueNotNull() || use->is_OpaqueTemplateAssertionPredicate() ||
> 326:                 use->is_OpaqueInitializedAssertionPredicate()) {

Code is executed with both new opaque nodes (hit during testing). Same below.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21608#discussion_r1808732859
PR Review Comment: https://git.openjdk.org/jdk/pull/21608#discussion_r1808753732
PR Review Comment: https://git.openjdk.org/jdk/pull/21608#discussion_r1808735284
PR Review Comment: https://git.openjdk.org/jdk/pull/21608#discussion_r1808736365
PR Review Comment: https://git.openjdk.org/jdk/pull/21608#discussion_r1808740425
PR Review Comment: https://git.openjdk.org/jdk/pull/21608#discussion_r1808745488