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

[Nils Eliasson]

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.

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