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

I know you explained it above in text form, but there should be an explanation at `make_paths_from_here_dead` to say why we cannot just put the `HaltNode` at the ctrl of the `CastII` for example. I think the idea is that the CastII could have its control hoisted, and so not all paths dominated by that control are actually dead... only those that use the CastII. It could be nice to have some concrete example, maybe some ASCII art showing the dead and live paths?

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

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