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

[Emanuel Peter]

On Fri, 7 Feb 2025 13:27:00 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 incrementally with one additional commit since the last revision:
> 
>   review

src/hotspot/share/opto/node.cpp line 3110:

> 3108: 
> 3109: Node* TypeNode::Ideal(PhaseGVN* phase, bool can_reshape) {
> 3110:   if (can_reshape && Value(phase) == Type::TOP) {

Why not use `phase->type(this)`?

test/hotspot/jtreg/compiler/c2/TestGuardOfCastIIDoesntFold.java line 30:

> 28:  * @run main/othervm -XX:-TieredCompilation -XX:-BackgroundCompilation -XX:-UseOnStackReplacement
> 29:  *                   -XX:CompileCommand=dontinline,TestGuardOfCastIIDoesntFold::notInlined
> 30:  *                   TestGuardOfCastIIDoesntFold

Nit: can we have a run without flags, please ;)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23468#discussion_r2005101901
PR Review Comment: https://git.openjdk.org/jdk/pull/23468#discussion_r2005044876