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

Wed Mar 3 11:25:13 UTC 2021

> 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-6 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

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2758/files
  - new: https://git.openjdk.java.net/jdk/pull/2758/files/90ac1bda..5cef73ad

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2758&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2758&range=00-01

  Stats: 18 lines in 3 files changed: 3 ins; 1 del; 14 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2758.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2758/head:pull/2758

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