RFR: 8238812: assert(false) failed: bad AD file
Christian Hagedorn
chagedorn at openjdk.java.net
Tue Mar 2 11:16:52 UTC 2021
On Fri, 26 Feb 2021 20:13:35 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-6 and pre-integeration tests
Changes requested by chagedorn (Reviewer).
src/hotspot/share/opto/ifnode.cpp line 876:
> 874: bool IfNode::get_base_comparing_value (Node* dom_if, PhaseIterGVN* igvn, jint& this_adj_val, jint& dom_adj_val) {
> 875: Node* this_cmp = in(1)->in(1)->as_Cmp();
> 876: Node* dom_cmp = dom_if->in(1)->in(1)->as_Cmp();
Could be directly turned into an assertion with `is_Cmp()` instead of using `as_Cmp()`.
src/hotspot/share/opto/ifnode.cpp line 882:
> 880: // Variant 1
> 881: return true;
> 882: } else if (this_val->is_Add() && this_val->in(1) != NULL && this_val->in(1) == dom_val) {
As `this_val` and `dom_val` are both non-null: `this_val->in(1) != NULL` is always true when `this_val->in(1) == dom_val` holds and thus `this_val->in(1) != NULL` can be removed. This can also be applied in Variant 3.
src/hotspot/share/opto/ifnode.cpp line 896:
> 894: return true;
> 895: }
> 896: } else if (this_val->is_Add() && dom_val->is_Add() && this_val->in(1) != NULL && dom_val->in(1) != NULL && this_val->in(1) == dom_val->in(1)) {
Here I think you can just remove one of the non-null checks (in case both are NULL).
src/hotspot/share/opto/ifnode.cpp line 860:
> 858: }
> 859:
> 860: // There might be an AddINode (marked with *) with a constant increment
Do we only need to do this for `AddNodes`? What about `SubNodes` for example?
src/hotspot/share/opto/ifnode.cpp line 641:
> 639: } else {
> 640: lo = TypeInt::INT->_lo;
> 641: }
Had some problems following this logic. But I think I got it now. What do you think about reformulating this (and the related comments further down) into
if (is_unsigned && lo >= 0) {
// cmp2 >= 0: val u<= cmp2 can only pass if val >= 0. Set val->_lo to 0.
} else {
// The lower bound of val cannot be improved.
lo = TypeInt::INT->_lo;
}
to make it more clear?
test/hotspot/jtreg/compiler/c2/TestJumpTable.java line 66:
> 64: // Original (slightly simplified) fuzzer generated test
> 65: public static void test1() {
> 66: int i4, i5=99, i6, i9=89;
Minor: Add spaces
src/hotspot/share/opto/ifnode.cpp line 723:
> 721:
> 722: // Is the comparison for this If suitable for folding?
> 723: bool IfNode::cmpi_folds(PhaseIterGVN* igvn) {
We should probably rename it into `cmp_folds` now that it is also for unsigned and also update the description above. Shouldn't the description be at `fold_compares()` anyways? But maybe we want to clean this up in a follow-up RFE.
-------------
PR: https://git.openjdk.java.net/jdk/pull/2758
More information about the hotspot-compiler-dev
mailing list