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

[Tobias Hartmann]

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
>>>