RFR: 8238812: assert(false) failed: bad AD file
Tobias Hartmann
thartmann at openjdk.java.net
Tue Mar 2 08:20:18 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 'fold_compares_helper()',
> - form new method
> (new 'fold_dominated_if()' added and renamed 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 CmpNodes 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 CmpNodes and the common value compared.
> (Sample IR extract with AddI in between CmpNodes and common value compared: )
> 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 thartmann (Reviewer).
src/hotspot/share/opto/ifnode.cpp line 672:
> 670: break;
> 671: default:
> 672: assert(false, "should not reach here");
Use `ShouldNotReachHere();` instead.
src/hotspot/share/opto/ifnode.cpp line 864:
> 862: // Check for the following cases. Then return common value compared
> 863: // if present and also save the constant values to be adjusted or
> 864: // subtracted due to the possible AddINode in-between.
I would replace `adjusted or subtracted` and say something like `... and also save the constant value that is added to infer the type of the common value we compare.`
src/hotspot/share/opto/ifnode.cpp line 881:
> 879: Node* res_val = NULL;
> 880: this_adj_val = 0;
> 881: dom_adj_val = 0;
The caller already initialized these to `0`, right?
src/hotspot/share/opto/ifnode.cpp line 877:
> 875: Node* this_cmp = in(1)->in(1);
> 876: Node* dom_cmp = dom_if->in(1)->in(1);
> 877: assert(this_cmp->is_Cmp() != false && dom_cmp->is_Cmp() != false, "compare expected");
No need to compare against false. This should be `assert(this_cmp->is_Cmp() && dom_cmp->is_Cmp(), ...` but instead you could also simply replace declarations above by `Node* this_cmp = in(1)->in(1)->as_Cmp()` which would include the assert.
src/hotspot/share/opto/ifnode.cpp line 885:
> 883: Node* this_val = this_cmp->in(1);
> 884: if (this_val == dom_val) {
> 885: res_val = this_val;
Add comments describing that this is Variant 1, same for other if branches below.
src/hotspot/share/opto/ifnode.cpp line 889:
> 887: const TypeInt* val_t = igvn->type(this_val->in(2))->isa_int();
> 888: if (val_t && val_t->is_con()) {
> 889: this_adj_val = val_t->_lo;
Replace `val_t->_lo` by `val_t->get_con()`.
src/hotspot/share/opto/ifnode.cpp line 886:
> 884: if (this_val == dom_val) {
> 885: res_val = this_val;
> 886: } else if (this_val->is_Add() && this_val->in(1) && this_val->in(1) == dom_val) {
`this_val->in(1)` implicitly casts a pointer to a boolean, should be `this_val->in(1) != NULL`. Same below.
src/hotspot/share/opto/ifnode.cpp line 890:
> 888: if (val_t && val_t->is_con()) {
> 889: this_adj_val = val_t->_lo;
> 890: res_val = this_val->in(1);
Could be replaced by `res_val = dom_val;`
src/hotspot/share/opto/ifnode.cpp line 888:
> 886: } else if (this_val->is_Add() && this_val->in(1) && this_val->in(1) == dom_val) {
> 887: const TypeInt* val_t = igvn->type(this_val->in(2))->isa_int();
> 888: if (val_t && val_t->is_con()) {
`val_t` implicitly casts a pointer to a boolean, should be replaced by `val_t != NULL`. Same below.
src/hotspot/share/opto/ifnode.cpp line 920:
> 918: Node* n = NULL;
> 919:
> 920: n = get_base_comparing_value(dom_if, igvn, this_adj_val, dom_adj_val);
No need to initialize `n` above. Should be `Node* n = get_base_comparing_value(...`
src/hotspot/share/opto/ifnode.cpp line 913:
> 911: // Check if dominating if determines the result of this if
> 912: bool IfNode::fold_dominated_if(ProjNode* proj, PhaseIterGVN* igvn) {
> 913: Node* this_cmp = in(1)->in(1);
We only ever use `this_cmp->in(1)` so you can replace this by `Node* this_val = in(1)->in(1)->in(1)`. Same for `dom_cmp` below.
src/hotspot/share/opto/ifnode.cpp line 934:
> 932: }
> 933: for (int i = 0; i < 2; ++i) {
> 934: const TypeInt* type2 = filtered_int_type(igvn, this_cmp->in(1), proj_out(i));
Why not just name this `type`?
src/hotspot/share/opto/ifnode.cpp line 941:
> 939: type2 = this_cmp->in(1)->as_Add()->add_ring(type2, TypeInt::make(-this_adj_val))->is_int();
> 940: }
> 941: const TypeInt* res_type = failtype->join(type2)->is_int();
No need to declare `res_type`, just use `type`.
-------------
PR: https://git.openjdk.java.net/jdk/pull/2758
More information about the hotspot-compiler-dev
mailing list