RFR: 8350563: C2 compilation fails because PhaseCCP does not reach a fixpoint [v8]

[Christian Hagedorn]

On Sat, 5 Apr 2025 18:51:35 GMT, Liam Miller-Cushon <cushon at openjdk.org> wrote:

>> Hello, please consider this fix for [JDK-8350563](https://bugs.openjdk.org/browse/JDK-8350563) contributed by my colleague Matthias Ernst.
>> 
>> https://github.com/openjdk/jdk/pull/22856 introduced a new `Value()` optimization for the pattern `AndIL(Con, Mask)`.
>> This optimization can look through CastNodes, and therefore requires additional logic in CCP to push these
>> transitive uses to the worklist.
>> 
>> The optimization is closely related to analogous optimizations for SHIFT nodes, and we also extend the existing logic for
>> CCP worklist handling: the current logic is "if the shift input to a SHIFT node changes, push indirect AND node uses to the CCP worklist".
>> We extend it by adding "if the (new) type of a node is an IntegerType that `is_con, ...` to the predicate.
>
> Liam Miller-Cushon has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 15 additional commits since the last revision:
> 
>  - Add -XX:+UnlockDiagnosticVMOptions
>  - Check type before uncasting
>    
>    A child phi node may transition from con to non-con, making the AND node transition back from "0" to its current type. If that current type is still TOP we're in violation of monotonicity. Therefore, don't apply optimization if AND is not integer yet.
>  - Merge commit '9bb804b14e1' into JDK-8350563
>  - Explicitly check for OP_Con instead of TypeInteger::is_con.
>    
>     322  Phi  === 303 119 255  [[ 399 388 351 751 366 377 ]]  #int:-256..127 !jvms: Integer::parseInt @ bci:151 (line 625)
>    
>     While this Phi dumps as "#int:-256..127", `phase->type(expr)` returns a type that is_con -256.
>  - Update test/hotspot/jtreg/compiler/ccp/TestAndConZeroCCP.java
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - Merge remote-tracking branch 'origin/master' into JDK-8350563
>  - Reformat test and update package to ccp
>  - Review comments
>  - Update test/hotspot/jtreg/compiler/c2/TestAndConZeroCCP.java
>    
>    Co-authored-by: Emanuel Peter <emanuel.peter at oracle.com>
>  - copyright
>  - ... and 5 more: https://git.openjdk.org/jdk/compare/c9e08477...23119e18

Sure, you're welcome! The fix looks good to me now. I will emit some internal testing again when you merged the tests together. Then I think it's good to go :-)

test/hotspot/jtreg/compiler/ccp/TestAndConZeroMonotonic.java line 35:

> 33: public class TestAndConZeroMonotonic {
> 34: 
> 35:     public static void main(String[] args) {

Since it's quite an easy test, I suggest to merge the two test files together by calling `Integer::parseInt()` directly instead. You can just add another `@run` statement.

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23871#pullrequestreview-2745711197
PR Review Comment: https://git.openjdk.org/jdk/pull/23871#discussion_r2030575931