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

[Vladimir Kozlov]

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.

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