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

[Vladimir Kozlov]

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...

Looks good.

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

> 1371:     // Avoid RCE if Counted loop's test is '!='.
> 1372:     BoolTest::mask bt = cl->loopexit()->test_trip();
> 1373:     if (bt != BoolTest::lt && bt != BoolTest::gt)

Since you touching this code can you add `{}` for this condition?

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

Marked as reviewed by kvn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19500#pullrequestreview-2096777095
PR Review Comment: https://git.openjdk.org/jdk/pull/19500#discussion_r1626232817