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

[Christian Hagedorn]

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

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

> 2165:       // to avoid such a phi in between.
> 2166:       // For example, it is unexpected that there is a Phi between an
> 2167:       // AllocateArray node and its ValidLengthTest input that could cause

Was outdated (we already removed `Opaque1` nodes for predicates some time ago) and we also handle more nodes now and not only `AllocateArray`. Improved that a little bit.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19561#discussion_r1627801684