RFR: 8238812: assert(false) failed: bad AD file [v2]
Roland Westrelin
roland at openjdk.java.net
Fri Mar 5 16:02:21 UTC 2021
On Wed, 3 Mar 2021 11:25:13 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
>
> Rahul Raghavan has updated the pull request incrementally with one additional commit since the last revision:
>
> 8238812: assert(false) failed: bad AD file
Changes requested by roland (Reviewer).
src/hotspot/share/opto/ifnode.cpp line 646:
> 644: hi = hi - 1;
> 645: }
> 646: break;
I think there's a bug here (case BoolTest::lt, unsigned comparison) . if cmp2 is [-1, 1] for instance, then lo and hi are unchanged. If the type of val is TypeInt::INT then the resulting type once join is applied is the interval [-1, 1]. But for instance 42 <u -1, 42 is in TypeInt::INT, -1 is in [-1, 1] but 42 is not in the resulting interval. I wonder if other cases in this method has similar issues. Can you fix that one and verify others?
-------------
PR: https://git.openjdk.java.net/jdk/pull/2758
More information about the hotspot-compiler-dev
mailing list