[9] RFR(S): 8150804: C2 Compilation fails with assert(_base >= OopPtr && _base <= AryPtr) failed: Not a Java pointer

Tobias Hartmann tobias.hartmann at oracle.com
Tue Mar 15 07:01:08 UTC 2016


Thanks, Vladimir!

Best regards,
Tobias

On 14.03.2016 21:30, Vladimir Kozlov wrote:
> Looks good.
> 
> Thanks,
> Vladimir
> 
> On 3/14/16 8:21 AM, Tobias Hartmann wrote:
>> Hi,
>>
>> I triggered another problem with the latest webrev:
>>
>> #  Internal Error (/scratch/opt/jprt/T/P1/101046.tohartma/s/hotspot/src/share/vm/opto/phaseX.cpp:346), pid=11651, tid=11671
>> #  assert(t == t_no_spec) failed: dead node in hash table or missed node during speculative cleanup
>>
>> We should add the input nodes to the IGVN worklist after cutting off the Phi because they may be dead now:
>> http://cr.openjdk.java.net/~thartmann/8150804/webrev.02/
>>
>> Thanks,
>> Tobias
>>
>> On 14.03.2016 11:09, Tobias Hartmann wrote:
>>> Hi Vladimir,
>>>
>>> thanks for the review!
>>>
>>> On 14.03.2016 02:28, Vladimir Kozlov wrote:
>>>> It is strange that next subgraph did not collapse - CheckCastPPNode::Identity() should remove second CheckCastPP:
>>>>
>>>>>          CheckCastPP
>>>>>           A:NotNull
>>>>>           /       \
>>>>> CheckCastPP     |
>>>>>    A:NotNull      |
>>>>>            \     /
>>>>>              Phi
>>>>>               A
>>>
>>> When CheckCastPPNode::Identity() is first invoked, the graph looks like this
>>>
>>>   92    CheckCastPP    ===  90  32  [[]]  #A:NotNull *  Oop:A:NotNull * !jvms: TestPhiElimination::test @ bci:8
>>>   135    CheckCastPP    ===  133  92  [[]]  #A:NotNull * (speculative=A:NotNull:exact * (inline_depth=2))  Oop:A:NotNull * (speculative=A:NotNull:exact * (inline_depth=2)) !jvms: A::get @ bci:9 TestPhiElimination::test @ bci:11
>>>
>>> Because 135 has a speculative part, the types are not equal and 135 is not replaced by 92.
>>>
>>>> I think next code in PhiNode::Ideal() should check can_reshape too since during parsing graph is incomplete:
>>>>
>>>>    if (uin == NULL) {
>>>>      uncasted = true;
>>>>      uin = unique_input(phase, true);
>>>>    }
>>>
>>> Yes, that's a better solution. I verified that it works (Phi is folded after parsing and has correct A:NotNull type).
>>>
>>> Here is the new webrev:
>>> http://cr.openjdk.java.net/~thartmann/8150804/webrev.01/
>>>
>>> Thanks,
>>> Tobias
>>>
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 3/11/16 7:40 AM, Tobias Hartmann wrote:
>>>>> Hi,
>>>>>
>>>>> please review the following patch.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8150804
>>>>> http://cr.openjdk.java.net/~thartmann/8150804/webrev.00/
>>>>>
>>>>> We fail in Compile::Process_OopMap_Node() while processing monitors of a safepoint node because the monitor object is TOP. The crash is rare but reproduces with my regression test. The problem is the elimination of Phi nodes with a unique input which was broken by the fixes for JDK-8139771 [1] and JDK-8146999 [2].
>>>>>
>>>>> Here are the details (for context, see 'TestPhiElimination.java'):
>>>>> A::get() is inlined into test(obj) producing the following graph:
>>>>>
>>>>>           Parm (obj)
>>>>>        TestPhiElimination
>>>>>               |
>>>>>             CastPP
>>>>>    TestPhiElimination:NotNull
>>>>>               |
>>>>>          CheckCastPP
>>>>>           A:NotNull
>>>>>           /       \
>>>>> CheckCastPP     |
>>>>>    A:NotNull      |
>>>>>            \     /
>>>>>              Phi
>>>>>               A
>>>>>               |
>>>>>           Safepoint
>>>>>
>>>>> https://bugs.openjdk.java.net/secure/attachment/57820/before_ideal.png
>>>>>
>>>>> PhiNode::ideal() then replaces the Phi by a CheckCastPP because it has a unique input (see PhiNode::unique_input()):
>>>>>
>>>>>           Parm (obj)
>>>>>        TestPhiElimination
>>>>>               |
>>>>>          CheckCastPP
>>>>>               A
>>>>>               |
>>>>>           Safepoint
>>>>>
>>>>> https://bugs.openjdk.java.net/secure/attachment/57821/after_ideal.png
>>>>>
>>>>> We completely lose the NotNull information provided by the CastPP. Therefore, we cannot prove that obj != null when accessing a field of obj and add an uncommon trap. Obj is also used as a monitor (A::get() is synchronized) and set to TOP in the uncommon trap branch. We are never able to prove that the null branch is not reachable and later fail when emitting code in Process_OopMap_Node because the monitor object is still TOP.
>>>>>
>>>>> Before the fix for JDK-8139771, we had a check to verify that the type of the unique (uncasted) input is "at least as good" as the type of the PhiNode:
>>>>>
>>>>>    phase->type(uncasted_input)->higher_equal(type()))
>>>>>
>>>>> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/rev/9e17d9e4b59f#l4.79
>>>>>
>>>>> Re-adding this check, fixes the problem. However, I'm concerned that this check is not strong enough. For example, in the case where the type of the PhiNode is Object:
>>>>>
>>>>>           Parm (obj)
>>>>>        TestPhiElimination
>>>>>               |
>>>>>             CastPP
>>>>>    TestPhiElimination:NotNull
>>>>>               |
>>>>>          CheckCastPP
>>>>>           A:NotNull
>>>>>           /       \
>>>>> CheckCastPP     |
>>>>>    A:NotNull      |
>>>>>            \     /
>>>>>              Phi
>>>>>             Object
>>>>>
>>>>> We would still replace the Phi because TestPhiElimination->higher_equal(Object) and again lose the NotNull information. I therefore added a slightly stronger check that also checks the types in-between. I had to remove the assert that Roland added.
>>>>>
>>>>> What do you think?
>>>>>
>>>>> Thanks,
>>>>> Tobias
>>>>>
>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8139771
>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8146999
>>>>>


More information about the hotspot-compiler-dev mailing list