RFR(S): 8205107: assert(c->Opcode() == Op_SafePoint) failed: broken outer loop

[Vladimir Kozlov]

On 6/20/18 12:22 PM, Nils Eliasson wrote:
> 
> 
> On 2018-06-20 18:32, Vladimir Kozlov wrote:
>> On 6/20/18 2:01 AM, Nils Eliasson wrote:
>>> I simplified it to:
>>>
>>>    virtual bool guaranteed_safepoint() {
>>>        // Calls to boxing methods will be expanded, and safepoints 
>>> aren't guaranteed
>>>        return !is_boxing_method();
>>>    }
>>>
>>> is_boxing_method() checks is_macro() that checks the Flag_is_macro 
>>> which is only set when C->eliminate_boxing() is true.
>>
>> Okay.
>>
>>>
>>> "(proj_out_or_null(TypeFunc::Parms) == NULL));" seems redundant too 
>>> me. What would that guard against?
>>
>> It checks that result of the call is not used (Box object is 
>> eliminated by EA). It is needed otherwise you will get bad graph.
> 
> We don't need that check since we prevent an optimization for happening. 
> The graph is kept as it is.

So the result is only used by Safepoint? Then how it is eliminated later?

Hmm, may be check _is_scalar_replaceable instead.

Vladimir

> 
> This is the graph:
> 
> Integer.valueOf
>      |
> CatchProj
>      |
> SafePoint
>      |
> OuterCountedLoopEnd
> 
> The original problem is that the SafePoint was removed because the 
> Integer.valueOf reported that it guarantees a safepoint. With the change 
> - guaranteed_safepoint() returns false for boxes - no safepoint removal 
> is done, the graph is kept intact.
> 
> Regards,
> 
> Nils
>>
>> Thanks,
>> Vladimir
>>
>>>
>>> New webrev here: http://cr.openjdk.java.net/~neliasso/8205107/webrev.03/
>>>
>>> Thanks for the help!
>>>
>>> Nils
>>>
>>>
>>>
>>> On 2018-06-19 19:18, Vladimir Kozlov wrote:
>>>> On 6/19/18 9:01 AM, Vladimir Kozlov wrote:
>>>>> On 6/19/18 7:22 AM, Nils Eliasson wrote:
>>>>>> I looked at it in a bit more detail.
>>>>>>
>>>>>> The call is a java/lang/Integer::valueOf that is considered a 
>>>>>> macro node so it is kept around until macro expansion which 
>>>>>> happens after the place where we hit the assert. So the call will 
>>>>>> go away.
>>>>>
>>>>> I assume you are talking about eliminate_boxing_node() in macro 
>>>>> expansion.
>>>>>
>>>>>>
>>>>>> However SafepointNode::Identity does check:
>>>>>>
>>>>>> if( n0->is_Call() && n0->as_Call()->guaranteed_safepoint() ) {
>>>>>>        // Useless Safepoint, so remove it
>>>>>>
>>>>>> And CallNode::guaranteed_safepoint() is:
>>>>>>
>>>>>>    // Are we guaranteed that this node is a safepoint? Not true 
>>>>>> for leaf calls and
>>>>>>    // for some macro nodes whose expansion does not have a 
>>>>>> safepoint on the fast path.
>>>>>>    virtual bool        guaranteed_safepoint()  { return true; }
>>>>>>
>>>>>> No check for call/macro-nodes is implemeted despite the comment! 
>>>>>> If I fix this the problem goes away.
>>>>>
>>>>> It returns false for Allocate nodes which are macro nodes. That is 
>>>>> what comment says.
>>>>>
>>>>> In case of boxing node I think we should add to CallStaticJavaNode 
>>>>> class
>>>>>
>>>>>    // Boxing call which is not used and will be removed.
>>>>>    virtual bool        guaranteed_safepoint()  { return 
>>>>> C->eliminate_boxing() && is_boxing_method() && 
>>>>> (proj_out_or_null(TypeFunc::Parms) == NULL); }
>>>>
>>>> Sorry, test should be reversed because it guarantee SP if call is 
>>>> *not* removed:
>>>>
>>>> virtual bool        guaranteed_safepoint()  { return 
>>>> !(C->eliminate_boxing() && is_boxing_method() && 
>>>> (proj_out_or_null(TypeFunc::Parms) == NULL)); }
>>>>
>>>> Vladimir
>>>>
>>>>>
>>>>> Vladimir
>>>>>
>>>>>>
>>>>>> // Nils
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2018-06-18 15:41, Roland Westrelin wrote:
>>>>>>>> Roland, why we need to keep Safepoint in Outer loop if we have 
>>>>>>>> call?
>>>>>>> I double checked with a simple test case and we don't keep the 
>>>>>>> safepoint
>>>>>>> if there's a call so it's indeed strange that we would need Nils' 
>>>>>>> patch.
>>>>>>>
>>>>>>> Roland.
>>>>>>
>>>
>