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

[Christian Hagedorn]

On Thu, 25 Apr 2024 13:34:31 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

src/hotspot/share/opto/loopPredicate.cpp line 353:

> 351:     }
> 352:     Node* bol = iff->in(1);
> 353:     assert(!bol->is_OpaqueInitializedAssertionPredicate(), "should not find an Initialized Assertion Predicate");

Initialized Assertion Predicates have halt nodes and not regions. Thus, we bail out on L350. Same argument for `Opaque4` nodes of non-null-checks, thus we assert on L355 that it's only for Template Assertion Predicates.

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

> 1203:       Node *bol = iff->in(1);
> 1204:       if (bol->req() < 2) {
> 1205:         continue; // dead constant test

Before: We bailed out for `Opaque4` nodes because they have 3 inputs. But comment suggests that this is only for dead constant tests. I therefore updated this and added assert below for `Opaque4` nodes.

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

> 1206:       }
> 1207:       if (!bol->is_Bool()) {
> 1208:         assert(bol->Opcode() == Op_Conv2B, "predicate check only");

Should have been removed before when we replaced `Opaque1->If` with `ParsePredicate`.

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

> 1207:       if (!bol->is_Bool()) {
> 1208:         assert(bol->is_Opaque4() || bol->is_OpaqueInitializedAssertionPredicate(),
> 1209:                "Opaque node of non-null-check or of Initialized Assertion Predicate");

We need this for Initialized `OpaqueInitializedAssertionPredicate` and `Opaque4` nodes for non-null-checks. This is covered by `testPolicyRangeCheck()`.

I did not try to come up with a test for Template Assertion Predicates. I think they cannot be found here since we eagerly remove them once they become useless. However, I'm planning to replace the `Opaque4` nodes for them anyways.

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

> 1990:         // Ignore Opaque4 from a non-null-check for an intrinsic or unsafe access. This could happen when we maximally
> 1991:         // unroll a non-main loop with such an If with an Opaque4 node directly above the loop entry.
> 1992:         assert(!loop_head->is_main_loop(), "Opaque4 node from a non-null check - should not be at main loop");

Covered by `testUnsafeAccess()`.

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

> 1986:     } else {
> 1987:       assert(b->is_Bool() || b->is_Opaque4() || b->is_OpaqueInitializedAssertionPredicate(),
> 1988:              "bool, non-null check with Opaque4 node or Initialized Assertion Predicate with its Opaque node");

Added case for Initialized Assertion Predicate. Covered by `testOpaqueInsideIfOutsideLoop()`.

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

> 2165:       // the AllocateArray node and its ValidLengthTest input that could cause
> 2166:       // split if to break.
> 2167:       if (use->is_If() || use->is_CMove() || use->is_Opaque4() ||

Added case for Initialized Assertion Predicate. Covered by `testOpaqueOutsideLoop()`.

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

> 2428:         assert(n->Opcode() == Op_LoopLimit ||
> 2429:                n->Opcode() == Op_Opaque3   ||
> 2430:                n->is_Opaque4()             ||

`OpaqueInitializedAssertionPredicate` are no macro nodes and are removed after loop opts. So, no changes required in this file.

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

> 323:           if (bol->outcnt() == 1) {
> 324:             Node* use = bol->unique_out();
> 325:             if (use->is_Opaque4() || use->is_OpaqueInitializedAssertionPredicate()) {

Added case for Initialized Assertion Predicate. Covered by `test*CloneDown*()`.

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

> 353:               Node* u = bol->out(j);
> 354:               // Uses are either IfNodes, CMoves, Opaque4, or OpaqueInitializedAssertionPredicates
> 355:               if (u->is_Opaque4() || u->is_OpaqueInitializedAssertionPredicate()) {

Added case for Initialized Assertion Predicate. Covered by `test*CloneDown*()`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18951#discussion_r1579483749
PR Review Comment: https://git.openjdk.org/jdk/pull/18951#discussion_r1579561681
PR Review Comment: https://git.openjdk.org/jdk/pull/18951#discussion_r1579508034
PR Review Comment: https://git.openjdk.org/jdk/pull/18951#discussion_r1579564368
PR Review Comment: https://git.openjdk.org/jdk/pull/18951#discussion_r1579619546
PR Review Comment: https://git.openjdk.org/jdk/pull/18951#discussion_r1579613455
PR Review Comment: https://git.openjdk.org/jdk/pull/18951#discussion_r1579614823
PR Review Comment: https://git.openjdk.org/jdk/pull/18951#discussion_r1579622648
PR Review Comment: https://git.openjdk.org/jdk/pull/18951#discussion_r1579617065
PR Review Comment: https://git.openjdk.org/jdk/pull/18951#discussion_r1579617313