RFR: 8333644: C2: assert(is_Bool()) failed: invalid node class: Phi

[Tobias Hartmann]

On Wed, 5 Jun 2024 13:33:27 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> In the patch of [JDK-8330386](https://bugs.openjdk.org/browse/JDK-8330386), I've added a test case that took the code path in `clone_loop_handle_data_uses()` with the new `OpaqueInitializedAssertionPredicateNode`. So, it proved that we should also add a case for `OpaqueInitializedAssertionPredicate` - yet somehow, I forgot to do that. Unfortunately, I could not come up with a failing test if we do not handle this case separately and the mistake went unnoticed.
> 
> But the fuzzer has now found such a case where we crash when creating the Mach graph because we have an `If` with a `Phi` input instead of a `Bool`. This happens exactly because of not handling `OpaqueInitializedAssertionPredicate` in  `clone_loop_handle_data_uses()`. 
> 
> We have an outside use of a `4843 Bool` (`4193 RangeCheck`) which is also an input into a `OpaqueInitializedAssertionPredicate` node:
> 
> ![image](https://github.com/openjdk/jdk/assets/17833009/7f43d987-6e29-449e-8392-58e76e111e80)
> 
> We should now clone this `Bool/Cmp` down to avoid having a `Phi` between the `OpaqueInitializedAssertionPredicate` and
> the `Bool` node which currently wrongly happens:
> 
> ![image](https://github.com/openjdk/jdk/assets/17833009/a10554a0-d48a-406c-a510-fef9666c45af)
> 
> and we crash later. With this simple patch, this is being avoided.
> 
> The good thing is that we now have a test that makes sure that this new condition is properly tested.
> 
> Thanks,
> Christian

Looks good to me.

test/hotspot/jtreg/compiler/predicates/assertion/TestOpaqueInitializedAssertionPredicateNode.java line 62:

> 60:  * @modules java.base/jdk.internal.misc:+open
> 61:  * @summary Test that using OpaqueInitializedAssertionPredicate for Initialized Assertion Predicates instead of Opaque4
> 62:  *          nodes also work with clone_loop_handle_data_uses() which missed a case before.

Suggestion:

 *          nodes also works with clone_loop_handle_data_uses() which missed a case before.

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19561#pullrequestreview-2099375442
PR Review Comment: https://git.openjdk.org/jdk/pull/19561#discussion_r1627889142