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

[Christian Hagedorn]

On Fri, 28 Mar 2025 15:32:30 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 five additional commits since the last revision:
> 
>  - review
>  - Merge branch 'master' into JDK-8349479
>  - review
>  - whitespace
>  - fix & test

Thanks for adding the `KillPathsReachableByDeadTypeNode` switch! Now that https://github.com/openjdk/jdk/pull/24246 is in, can you add `KillPathsReachableByDeadTypeNode` to the `TestAssertionPredicates.java` runs? To still run them with product, you can probably just use it together with `IgnoreUnrecognizedVMOptions`.

src/hotspot/share/opto/c2_globals.hpp line 824:

> 822:   develop(bool, KillPathsReachableByDeadTypeNode, true,                     \
> 823:           "When a Type node becomes top, make paths where the node is used" \
> 824:           "dead by replacing them with a Halt node")                        \

Maybe we can also add a warning of possible failures. Something like:
Suggestion:

  develop(bool, KillPathsReachableByDeadTypeNode, true,                     \
          "When a Type node becomes top, make paths where the node is "     \
          "used dead by replacing them with a Halt node. Turning this off " \
          "could corrupt the graph in rare cases and should be used with "  \
          "care.")                                                          \

src/hotspot/share/opto/castnode.cpp line 99:

> 97: // Return a node which is more "ideal" than the current node.  Strip out
> 98: // control copies
> 99: Node *ConstraintCastNode::Ideal(PhaseGVN *phase, bool can_reshape) {

While at it:
Suggestion:

Node* ConstraintCastNode::Ideal(PhaseGVN* phase, bool can_reshape) {

src/hotspot/share/opto/castnode.cpp line 103:

> 101:     return this;
> 102:   }
> 103:   if (in(1) != nullptr && phase->type(in(1)) != Type::TOP) {

Can `in(1)` ever be null?

src/hotspot/share/opto/convertnode.cpp line 732:

> 730: 
> 731: //------------------------------Ideal------------------------------------------
> 732: Node *ConvI2LNode::Ideal(PhaseGVN *phase, bool can_reshape) {

While at it:
Suggestion:

Node* ConvI2LNode::Ideal(PhaseGVN* phase, bool can_reshape) {

src/hotspot/share/opto/convertnode.cpp line 733:

> 731: //------------------------------Ideal------------------------------------------
> 732: Node *ConvI2LNode::Ideal(PhaseGVN *phase, bool can_reshape) {
> 733:   if (in(1) != nullptr && phase->type(in(1)) != Type::TOP) {

Same here and in `ConvL2I`, can `in(1)` ever be null?

src/hotspot/share/opto/convertnode.cpp line 841:

> 839: // Return a node which is more "ideal" than the current node.
> 840: // Blow off prior masking to int
> 841: Node *ConvL2INode::Ideal(PhaseGVN *phase, bool can_reshape) {

Suggestion:

Node* ConvL2INode::Ideal(PhaseGVN* phase, bool can_reshape) {

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

PR Review: https://git.openjdk.org/jdk/pull/23468#pullrequestreview-2728428324
PR Review Comment: https://git.openjdk.org/jdk/pull/23468#discussion_r2020526901
PR Review Comment: https://git.openjdk.org/jdk/pull/23468#discussion_r2020527365
PR Review Comment: https://git.openjdk.org/jdk/pull/23468#discussion_r2020528307
PR Review Comment: https://git.openjdk.org/jdk/pull/23468#discussion_r2020530780
PR Review Comment: https://git.openjdk.org/jdk/pull/23468#discussion_r2020531057
PR Review Comment: https://git.openjdk.org/jdk/pull/23468#discussion_r2020531623