RFR: 8290432: C2 compilation fails with assert(node->_last_del == _last) failed: must have deleted the edge just produced [v6]

[Christian Hagedorn]

On Mon, 21 Nov 2022 02:31:34 GMT, Yi Yang <yyang at openjdk.org> wrote:

>> Hi, can I have a review for this patch? [JDK-8273585](https://bugs.openjdk.org/browse/JDK-8273585) recognized the form of `Phi->CastII->AddI` as additional parallel induction variables. In the following program:
>> 
>> class Test {
>>     static int dontInline() {
>>         return 0;
>>     }
>> 
>>     static long test(int val, boolean b) {
>>         long ret = 0;
>>         long dArr[] = new long[100];
>>         for (int i = 15; 293 > i; ++i) {
>>             ret = val;
>>             int j = 1;
>>             while (++j < 6) {
>>                 int k = (val--);
>>                 for (long l = i; 1 > l; ) {
>>                     if (k != 0) {
>>                         ret += dontInline();
>>                     }
>>                 }
>>                 if (b) {
>>                     break;
>>                 }
>>             }
>>         }
>>         return ret;
>>     }
>> 
>>     public static void main(String[] args) {
>>         for (int i = 0; i < 1000; i++) {
>>             test(0, false);
>>         }
>>     }
>> }
>> 
>> `val` is incorrectly matched with the new parallel IV form:
>> ![image](https://user-images.githubusercontent.com/5010047/182059398-fc5204bc-8d95-4e3e-8c66-15776af457b8.png)
>> And C2 further replaces it with newly added nodes, which finally leads the crash:
>> ![image](https://user-images.githubusercontent.com/5010047/182059498-13148d46-b10f-4e18-b84a-f6b9f626ac7b.png)
>> 
>> I think we can add more constraints to the new form. The form of `Phi->CastXX->AddX` appears when using Preconditions.checkIndex, and it would be recognized as additional IV when 1) Phi != phi2, 2) CastXX is controlled by RangeCheck(to reflect changes in Preconditions checkindex intrinsic)
>
> Yi Yang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   whitespace

> If replacing Phi#125 is acceptable, we need to reach a consensus that the form of Add->CastII->Phi can be considered as parallel IV

I think it should be safe and we've already done that since JDK-8273585 and haven't seen a crash related to that idea. But given how close we are to the fork, I suggest to go with your bailout fix for JDK 20 which is safer (and performance testing done by Tobias looks good). In this way, we really only optimize the pattern originally intended in JDK-8273585.

For the general case of allowing any cast node, I suggest to file an RFE and investigate again if that is possible/correct. I have the feeling that it is but it would be better to defer that to JDK 21. What do you think?

Thanks,
Christian

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

Marked as reviewed by chagedorn (Reviewer).

PR: https://git.openjdk.org/jdk/pull/9695