RFR: 8238812: assert(false) failed: bad AD file [v4]

[Christian Hagedorn]

On Tue, 9 Mar 2021 14:35:27 GMT, Rahul Raghavan <rraghavan at openjdk.org> wrote:

>> Found that the earlier PR bug analysis and fix details done is wrong.
>> Request help to review this revised fix proposal.
>> 
>> Now understood need to fix two types of issues related to the bug-
>> 
>> (i). Crash with only `CmpI` nodes case.
>> Sample test case -
>> public static void test2() {
>>   for (int i = 5; i > -20; i -= 5) {
>>     switch (i) {
>>       case 0:
>>       case 10:
>>       case 20:
>>       case 30:
>>       case 40:
>>       case 50:
>>       case 60:
>>       case 100:
>>         res = 42;
>>         break;
>>     }
>>   }
>> }
>> 
>> // This generates the following subgraph:
>> /*
>> // i: -20..0
>> if (i != 0) {
>>   // i: -20..-1
>>   if (i < 0) {   <- /*This is always true but not folded by C2*/
>>     // Fall through
>>   } else {
>>     ...
>>     CastII(i-1, 0..4) <- /*Replaced by TOP because i-1 range is -21..-1 but still considered reachable by C2 although it is dead code*/
>>     ...
>>   }
>> } else {
>>   StoreI   <- Due to this additional store on, IfNode::has_shared_region returns false and the fold compares optimization does not kick in
>> }
>> */
>> 
>> Sample IR extracts -
>>  77  Phi  ===  399  25  389  [[ 658  664  389  80  662  660  659 ]]
>>  25  ConI  ===  0  [[ 80  77 ]]  #int:0
>>  80  CmpI  === _  77  25  [[ 81  89 ]]
>>  81  Bool  === _  80  [[ 82 ]] [ne]
>>  82  If  ===  399  81  [[ 85  87 ]]
>>  85  IfFalse  ===  82  [[ 102  336 ]]
>>  87  IfTrue  ===  82  [[ 90 ]] #1
>>  89  Bool  === _  80  [[ 90 ]] [lt]
>>  90  If  ===  87  89  [[ 91  92 ]]
>>  91  IfTrue  ===  90  [[ 102 ]] #1
>>  92  IfFalse  ===  90  [[ 156 ]] #0
>>  336  StoreI  ===  85  359  335  332  [[ 337 ]]
>>  156  Jump  ===  92  1  [[ 157  160  .......
>> 
>> Fix proposal here includes :
>> - Extract the existing non-`CmpU` related optimization out of old `fold_compares_helper()`,
>> - form new method
>> (new `fold_dominated_if()` added and renamed old `fold_compares_helper` as `fold_to_unsigned()`).
>> - Call it properly from `fold_compares()` and related code adjustments.
>> 
>> 
>> 
>> 
>> (ii). `fold_compares()` not handling `CmpU` nodes cases.
>> 
>> Sample basic test case -
>> public static void test3() {
>>   int local = 0;
>>   for (int i = 5; i > -20; i -= 5) {
>>     switch (i) {
>>     case 0:
>>     case 10:
>>     case 20:
>>     case 30:
>>     case 40:
>>     case 50:
>>     case 100:
>>       local = 42;
>>       break;
>>     }
>>   }
>> }
>> 
>> // This generates the following subgraph:
>> /*
>> // i: -20..0
>> if (i != 0) {
>>   // i: -20..-1
>>   if (i u< 101) {    <- /*This is always false but not folded by C2 because CmpU is not handled*/
>>     CastII(i-1, 0..49)  <- /*Replaced by TOP because i-1 range is -21..-1 but still considered reachable by C2 although it is dead code*/
>>   } else {
>>     ...
>>   }
>> } else {
>>   ...
>> }
>> */
>> 
>> Sample IR extracts -
>>  77  Phi  ===  363  20  353  [[ 573  577  572  80  575  353  345  574 ]]
>>  20  ConI  ===  0  [[ 80  77 ]]  #int:0
>>  80  CmpI  === _  77  20  [[ 81 ]]
>>  81  Bool  === _  80  [[ 82 ]] [ne]
>>  82  If  ===  363  81  [[ 85  87 ]]
>>  85  IfFalse  ===  82  [[ 102 ]] #0
>>  87  IfTrue  ===  82  [[ 337 ]] #1
>>  95  ConI  ===  0  [[ 345 ]]  #int:101
>>  345  CmpU  === _  77  95  [[ 346 ]]
>>  346  Bool  === _  345  [[ 337 ]] [lt]
>>  337  If  ===  87  346  [[ 338  339 ]]
>>  338  IfFalse  ===  337  [[ 102 ]] #0
>>  339  IfTrue  ===  337  [[ 135 ]] #1
>>  135  Jump  ===  339  1  [[ 136  139  142  .....
>> 
>> Proposed fix includes:
>> Enabling `Op_CmpU` support in -
>> - `cmpi_folds()`,
>> - `filtered_int_type()` - returns possible restrictive type for input value based on condition control flow for the if (supported `CmpU` for various condition control types).
>> - `fold_dominated_if()` - checks if dominating if determines the result of current if and above example test case is a basic variant possibility where both `CmpNode`s compare a common value directly. (`Node 77 Phi` in above sample). But as mentioned in the comments of new helper function `get_base_comparing_value()`, there are other possibilities where there might be an `AddINode` with a constant increment present in between the `CmpNode`s and the common value compared. 
>> (Sample IR extract with `AddI` in between `CmpNode`s :  )
>>  76  Phi  ===  364  79  354  [[ 576  580  575  80  578  354  344  577 ]]  #int:0..20:www
>>  79  ConI  ===  0  [[ 80  76 ]]  #int:20
>>  80  CmpI  === _  76  79  [[ 81 ]]
>>  81  Bool  === _  80  [[ 82 ]] [ne] 
>>  82  If  ===  364  81  [[ 85  87 ]]
>>  85  IfFalse  ===  82  [[ 102 ]] #0
>>  87  IfTrue  ===  82  [[ 338 ]] #1
>>  343  ConI  ===  0  [[ 344 ]]  #int:-20
>>  344  AddI  === _  76  343  [[ 348 ]] 
>>  347  ConI  ===  0  [[ 348 ]]  #int:281
>>  348  CmpU  === _  344  347  [[ 349 ]] 
>>  349  Bool  === _  348  [[ 338 ]] [lt]
>>  338  If  ===  87  349  [[ 339  340 ]]
>>  339  IfFalse  ===  338  [[ 102 ]] #0 
>>  340  IfTrue  ===  338  [[ 136 ]] #1
>>  136  Jump  ===  340  1  [[ 137  140  143  146 .........
>> All these variants are handled in `get_base_comparing_value()` and related test cases added.
>> 
>> - and related changes in `fold_compares()`.
>> 
>> 
>> Found no issues with this proposed fix for various unit tests, reported java_fuzzer case, tier1-7 and pre-integeration tests
>
> Rahul Raghavan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8238812: assert(false) failed: bad AD file

Looks good!

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

Marked as reviewed by chagedorn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2758