RFR: 8333252: C2: assert(assertion_predicate_has_loop_opaque_node(iff)) failed: must find OpaqueLoop* nodes

[Christian Hagedorn]

On Fri, 31 May 2024 12:33:04 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> [JDK-8330386](https://bugs.openjdk.org/browse/JDK-8330386) added some additional asserts to ensure that we are dealing with Template Assertion Predicates and not non-null-checks which both use `Opaque4` nodes.
> 
> #### Correct Assertion
> One of this assert was now hit with a fuzzer found case in `get_assertion_predicates()` called during the elimination of useless predicates. We walk through all loops and collect all useful Template Assertion Predicates and Parse Predicates above the loops. For that we look at the UCTs which are shared among the predicates. When finding a predicate with such an UCT which also has an `Opaque4` node, we know that it is a Template Assertion Predicate. We additionally assert that we must find the `OpaqueLoop*Nodes` above which always belong to a template:
> 
> https://github.com/openjdk/jdk/blob/7ab74c5f268dac82bbd36355acf8e4f3d357134c/src/hotspot/share/opto/loopPredicate.cpp#L346-L354
> 
> So, this assert looks correct.  
> 
> #### Why didn't we find `OpaqueLoop*Nodes` in this case?
> For the Template Assertion Predicate for the last value, we insert an additional `CastII` to keep the type information of the iv phi: 
> 
> https://github.com/openjdk/jdk/blob/7ab74c5f268dac82bbd36355acf8e4f3d357134c/src/hotspot/share/opto/loopPredicate.cpp#L1323-L1324
> 
> But in the test case, the type of the iv phi is a constant (`521 CastII`):
> 
> ![image](https://github.com/openjdk/jdk/assets/17833009/5dc17b9c-abfe-4846-89a1-4e189234b991)
> 
> `521 CastII` will simply be replaced with a constant during IGVN and the `OpaqueLoop*Nodes` above are removed. We therefore cannot find them anymore later when trying to eliminate useless predicates and we hit the assert.
> 
> #### Why does the `CastII`/iv phi have a constant type?
> Having a constant type for the iv phi indicates that the counted loop is only going to be executed for one iteration. But C2 has not had the chance, yet, to fold the loop exit test to remove the loop.
> 
> #### How to fix this bug?
> Having a single iteration loop raises the question, why we even bother to try and hoist checks out of such a loop with Loop Predication in the first place. I therefore suggest to simply bail out of Loop Predication if the trip count is 1. This will also prevent us from creating a Template Assertion Predicate with a `CastII` with a constant type from the iv phi which would be folded.
> 
> To do that, we can compute the trip count on entry of Loop Predication. By doing that, we can also remove the trip count computation added for hoisting ran...

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

> 1364:     }
> 1365:     loop->compute_trip_count(this);
> 1366:     if (cl->trip_count() == 1) {

I don't check for `has_exact_trip_count()` here since we could also have non-`ConNodes` with types such that the iv phi is still a constant and we know that we will only iterate for at most one iteration.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19500#discussion_r1622359484