RFR: 8373525: C2: assert(_base == Long) failed: Not a Long [v2]

[Christian Hagedorn]

On Fri, 19 Dec 2025 15:11:49 GMT, Damon Fenacci <dfenacci at openjdk.org> wrote:

>> # Issue
>> Olivier's fuzzer found a test that makes C2 crash while running the optimization that collapses the addition with overflow-protection (`fold_subI_no_underflow_pattern`).
>> 
>> # Causes
>> The crash happens because during `fold_subI_no_underflow_pattern` the first input of the `AddL` node (see comment below) becomes top.
>> https://github.com/openjdk/jdk/blob/82b04f01bc99e8155518b8b8600d180981a42fc5/src/hotspot/share/opto/addnode.cpp#L1525-L1533
>> 
>> This happens because of a whole `IfFalse` subgraph that dies and nodes are being removed. `AddL` is not removed immediately as it has another input which is still alive but it is put in the IGVN worklist instead.
>> 
>> <img width="463" height="239" alt="image" src="https://github.com/user-attachments/assets/bce0e4b0-b823-473d-91de-2bb048841e65" />
>> 
>> Unfortunately the `fold_subI_no_underflow_pattern` optimization runs before the next GVN pass and triggers the assert.
>> 
>> # Fix
>> `fold_subI_no_underflow_pattern` should actually take into account that we could have the graph in such a state and that `x` could be top. So, the sensible fix is not to presume `x` to be of type long and bailout if it is not.
>> 
>> # Testing
>> Tier 1-3+
>> (also checked for new regression test failure before the change)
>
> Damon Fenacci has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Update test/hotspot/jtreg/compiler/loopopts/TestValidTypeInOverflowProtection.java
>    
>    Co-authored-by: Manuel Hässig <manuel at haessig.org>
>  - JDK-8373525: remove test requires

Otherwise, looks good to me, too, thanks!

src/hotspot/share/opto/addnode.cpp line 1603:

> 1601:           const TypeLong* x_long = phase->type(x)->isa_long();
> 1602:           // Collapsed graph not equivalent if potential over/underflow -> bailing out (*)
> 1603:           if (x_long == nullptr || can_overflow(x_long, con1->get_long() + con2->get_long())) {

I suggest to to add a comment about when `x_long` is not a long as described in the PR description.

test/hotspot/jtreg/compiler/loopopts/TestValidTypeInOverflowProtection.java line 30:

> 28:  * @run main/othervm -Xbatch -XX:-TieredCompilation -XX:CompileCommand=compileonly,compiler.loopopts.TestValidTypeInOverflowProtection::test
> 29:  *                   ${test.main.class}
> 30:  * @run driver ${test.main.class}

Should be `main` to allow to pass flags in.

Suggestion:

 * @run main ${test.main.class}

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

Changes requested by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/28920#pullrequestreview-3599181977
PR Review Comment: https://git.openjdk.org/jdk/pull/28920#discussion_r2635589252
PR Review Comment: https://git.openjdk.org/jdk/pull/28920#discussion_r2635590558