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

[Tobias Hartmann]

On Thu, 3 Dec 2020 06:51:12 GMT, Rahul Raghavan <rraghavan at openjdk.org> wrote:

>> Request help to review this 8238812 fix.
>> 
>> <JBS> https://bugs.openjdk.java.net/browse/JDK-8238812
>> 
>> Found the root cause of the problem as that the following type 
>> two If checks are not folded into one for the sample test:
>> 
>>  170 ConI === 0 [[ 171 ]] #int:31
>>  171 CmpI === _ 144 170 [[ 172 179 ]]
>>  172 Bool === _ 171 [[ 173 ]] [ne]
>> 
>>  173 If === 158 172 [[ 176 177 ]]
>>  176 IfFalse === 173 [[ 168 ]] #0
>>  177 IfTrue === 173 [[ 180 ]] #1
>>  179 Bool === _ 171 [[ 180 ]] [lt]
>> 
>>  180 If === 177 179 [[ 181 182 ]]
>>  181 IfTrue === 180 [[ 168 ]] #1
>>  182 IfFalse === 180 [[ 218 ]] #0
>> 
>> Where 144 is the value and the ifs check for:
>> value != 31
>> value < 31
>> 
>> Now if we reach 182 IfFalse, the following holds:
>>    value != 31 && !(value < 31)
>> => value != 31 && value >= 31
>> => value > 31
>> 
>> But the type of value (144) becomes #int:16..31 :
>>  140 ConI === 0 [[ 141 ]] #int:2
>>  135 Phi === 534 481 528 [[ ... ]] #int:1..4 #tripcount
>>  141 LShiftI === _ 135 140 [[ 142 ]] #int:4..16
>>  143 ConI === 0 [[ 144 ]] #int:11
>>  142 AddI === _ 141 135 [[ 144 500 485 ]] #int:5..20
>>  144 AddI === _ 142 143 [[ ... ]] #int:16..31
>> 
>> And that means that value > 31 is always false and should be folded.
>> The root cause of the bug is that this does not happen.
>> Understood that the fix by Tobias for JDK-8236721 should optimize exactly above type cases
>> https://hg.openjdk.java.net/jdk/jdk/rev/9c53fdf6ba63
>> 
>> Found the IfNode::fold_compares optmization (and 8236721 related changes by Tobias)
>> is not called for 180-If, after the type of 144-AddI is set to #int:16..31.
>> because the _worklist is not correctly updated!
>> 
>> Proposed changes helped to add the required If node to _worklist (after the correct type setting of related value)
>> and with this change no failure due to existing fold_compares optimization happening later on.
>> Sample test passed and found no issues with various tier testing.
>
> Rahul Raghavan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8238812: assert(false) failed: bad AD file

Otherwise looks good.

src/hotspot/share/opto/phaseX.cpp line 1557:

> 1555:         // Put If on worklist to enable fold_compares optimization (see IfNode::cmpi_folds)
> 1556:         if (use->outcnt() > 0) {
> 1557:           for (uint i = 0; i < use->outcnt(); i++) {

The `use->outcnt() > 0` check is redundant and you could use `DUIterator_Fast` instead of `raw_out`. Also, `bol` could have multiple if users. For an example, see code in `countedloop_phi_from_cmp`.

test/hotspot/jtreg/compiler/c2/Test8238812.java line 29:

> 27:  * @summary Fix c2 assert(false) failed: bad AD file
> 28:  * @run main/othervm -XX:CompileCommand=compileonly,compiler.c2.Test8238812::test
> 29:  *                   -XX:CompileCommand=dontinline,compiler.c2.Test8238812::test

I don't think the `dontinline` statement is required if we are only compiling that method anyway, right?

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

Marked as reviewed by thartmann (Reviewer).

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