RFR: 8325494: C2: Broken graph after not skipping CastII node anymore for Assertion Predicates after JDK-8309902

[Christian Hagedorn]

On Wed, 10 Apr 2024 13:41:11 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> After range check elimination, a cast in the main loop becomes top
> because the type of its input (that depends on the iv phi) and the
> type recorded in the cast do not intersect. This is a case that's
> expected to be caught by assert predicates but, in this particular
> case, no assert predicate constant folds.
> 
> The stride for the loop is -2. The iv phi type is `min+1..0`
> 
> As a consequence, the init value for the main loop has type int.
> 
> The range check that causes the issue is for array access:
> 
> lArrFld[i11 + 1] = 6;
> 
> 
> The main loop is unrolled once. The second access in the loop is at
> `i11 - 1` which has type `min..-1`. The range check cast at that
> access becomes top. The assert predicates operates on an init value
> that has the shape:
> 
> 
> (CastII (AddI pre_loop_iv -2) int)
> 
> 
> and type int.
> 
> That `CastII` is inserted by `PhaseIdealLoop::cast_incr_before_loop()`.
> 
> The assert predicate for the first iteration in the main loop is for
> index:
> 
> 
> (AddI (CastII (AddI pre_loop_iv -2) int) 1)
> 
> 
> And for the second:
> 
> 
> (AddI (CastII (AddI pre_loop_iv -2) int) -1)
> 
> 
> Both have type int so the assert predicate can't constant fold.
> 
> I initially fixed this by changing the type of the cast from int to
> the type of the iv phi:
> 
> 
> (AddI (CastII (AddI pre_loop_iv -2) min+1..0) -1)
> 
> 
> That allows the assert predicate for the second iteration to constant
> fold. But I was then worried narrowing the type of the cast would
> causes issues going forward so instead, I propose proceeding as in
> 8282592 and have assert predicates skip over the CastII (that part of
> 8282592 was later undone):
> 
> 
> (AddI (AddI pre_loop_iv -2) 1)
> 
> 
> which allows the assert predicate for the first iteration in the main
> loop to constant fold.
> 
> The change from 8282592 caused issues because we used to narrow the
> type of a cast based on the condition that guards it. That was removed
> by 8319372.

I agree with that. I thought about same now that JDK-8319372 is in.

Two minor comments, otherwise, looks good!

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

> 2017:     // skip over the cast added by PhaseIdealLoop::cast_incr_before_loop() when pre/post/main loops are created because
> 2018:     // it can get in the way of type propagation
> 2019:     assert(((CastIINode*)init)->carry_dependency() && loop_head->skip_assertion_predicates_with_halt() == init->in(0), "casted iv phi from pre loop expected");

You can use `is_CastII()` and `as_CastII()`.

test/hotspot/jtreg/compiler/loopopts/TestAssertPredicateDoesntConstantFold.java line 32:

> 30:  */
> 31: 
> 32: public class TestAssertPredicateDoesntConstantFold {

I suggest to move this test to the other predicate tests in `compiler/predicates`

Nit:
Suggestion:

public class TestAssertionPredicateDoesntConstantFold {

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18724#pullrequestreview-1991798819
PR Review Comment: https://git.openjdk.org/jdk/pull/18724#discussion_r1559506640
PR Review Comment: https://git.openjdk.org/jdk/pull/18724#discussion_r1559508534