RFR: 8349479: C2: when a Type node becomes dead, make CFG path that uses it unreachable [v9]

[Christian Hagedorn]

On Tue, 8 Apr 2025 12:34:01 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> This is primarily motivated by 8275202 (C2: optimize out more
>> redundant conditions). In the following code snippet:
>> 
>> 
>> int[] array = new int[arraySize];
>> if (j <= arraySize) {
>>   if (i >= 0) {
>>     if (i < j) {
>>       int v = array[i]; 
>> 
>> 
>> (`arraySize` is a constant)
>> 
>> at the range check, `j` is known to be in `[min, arraySize]` as a
>> consequence, `i` is known to be `[0, arraySize-1]`. The range check
>> can be eliminated.
>> 
>> Now, if later, `i` constant folds to some value that's positive but
>> out of range for the array:
>> 
>> - if that happens when the new pass runs, then it can prove that:
>> 
>>     if (i < j) {
>> 
>> is never taken.
>> 
>> - if that happens during IGVN or CCP however, that condition is not
>>   constant folded. And because the range check was removed, there's no
>>   guard protecting the range check `CastII`. It becomes `top` and, as
>>   a result, the graph can become broken.
>>   
>> What I propose here is that when the `CastII` becomes dead, any CFG
>> paths that use the `CastII` node is made unreachable. So in pseudo code:
>> 
>> 
>> int[] array = new int[arraySize];
>> if (j <= arraySize) {
>>   if (i >= 0) {
>>     if (i < j) {
>>       halt();
>> 
>> 
>> Finding the CFG paths is implemented in the patch by following the
>> uses of the node until a CFG node or a `Phi` is encountered.
>> 
>> The patch applies this to all `Type` nodes as with 8275202, I also ran
>> in some rare corner cases with other types of nodes. The exception is
>> `Phi` nodes which may not be as easy to handle (and for which I had no
>> issue with 8275202).
>> 
>> Finally, the patch includes a test case that's unrelated to the
>> discussion of 8275202 above. In that test case, a `CastII` becomes top
>> but the test that guards it doesn't constant fold. The root cause is a
>> transformation of:
>> 
>> 
>> (CastII (AddI
>> 
>> 
>> into 
>> 
>> 
>> (AddI (CastII ) (CastII)`
>> 
>> 
>> which causes the resulting node to have a wider type. The `CastII`
>> captures a type before the transformation above happens. Once it has
>> happened, the guard for the `CastII` can't be constant folded when an
>> out of bound value occurs.
>> 
>> This is likely fixable some other way (eventhough it doesn't seem
>> straightforward). Given the long history of similar issues (and the
>> test case that shows that they are more hiding), I think it would
>> make sense to try some other way of approaching them.
>
> Roland Westrelin 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 17 additional commits since the last revision:
> 
>  - review
>  - Merge branch 'master' into JDK-8349479
>  - Update src/hotspot/share/opto/node.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - Update test/hotspot/jtreg/compiler/c2/TestGuardOfCastIIDoesntFold.java
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - review
>  - review
>  - review
>  - Merge branch 'master' into JDK-8349479
>  - Update src/hotspot/share/opto/convertnode.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - Update src/hotspot/share/opto/convertnode.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - ... and 7 more: https://git.openjdk.org/jdk/compare/8912f806...d9f53010

Testing looked good!

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

PR Comment: https://git.openjdk.org/jdk/pull/23468#issuecomment-2788380903