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

[Liam Miller-Cushon]

On Wed, 19 Mar 2025 16:18:33 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 incrementally with one additional commit since the last revision:
> 
>   Update test/hotspot/jtreg/compiler/ccp/TestAndConZeroCCP.java
>   
>   Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>

>From Matthias

---

I was able to reproduce the issue as is by Christian and I have a fix - with some caveats since I only have a partial understanding of what's happening.

Here's what I know:

In order to simplify EXPR & MASK, AndINode::Value() compares the trailing zero bits of EXPR against the width of MASK.
For this, we use phase->type(expr)->is_con().

What I observe for the Integer.parseInt reproducer is that `expr` _dumps_ as a phi node with type #int:-256...127, but phase->type(expr) returns a type that is_con() with value -256.
In consequence, the AND(phi-node, mask) gets optimized to zero.

Concretely, my understanding is that the node is "digit" here:
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/Integer.java#L618,L622,L627
618        int digit = ~0xFF;                                     lo = -256
622 ..if..  { digit = digit(firstChar, radix); .. }            assumes Latin1 => byte => hi = 127 from here: https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/CharacterDataLatin1.java.template#L233
627:      int result = -(digit & 0xFF);

I can only guess why we'd get an is_con() type for this node, I assume it's a speculative optimization, but I would expect that to happen all the time.
Why reducing this phi node to zero would cause an issue, though, I'm out of my depth there.

Anyway, my first instinct is to replace the constant check in the optimization `phase->type(expr)->is_con()` with an explicit opcode check (== OP_ConI || OP_ConL).
This a) fixes the crash, b) passes all tests we added/changed the new optimization, so it doesn't undo anything we were trying to accomplish in the first place.
I've pushed a corresponding commit, ptal:

-  if (type->is_con()) {
+  if (expr->Opcode() == Op_ConI || expr->Opcode() == Op_ConL) {


It does feel like it's addressing a symptom though, not a cause.
That being said, the "And[IL]Node::Value" optimization for "const & mask => 0" has always been a "happy byproduct", the actual goal was always the optimization in "::Ideal": "((expr + const) << shift) & mask => expr & mask",
which still works. LMK what you think.

Unfortunately, I have been unable to reproduce this outside of jl.Integer.parseInt. Even when copying jl.Integer.parseInt to my own method, I wasn't able to trigger the crash.

Matthias

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

PR Comment: https://git.openjdk.org/jdk/pull/23871#issuecomment-2766699676