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

[Nils Eliasson]

On 2018-06-20 21:33, Vladimir Kozlov wrote:
> 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?

It has other uses also. I'm not sure why the call is left, I will 
investigate it further and can file a seperate bug if it is something wrong.

For this bug the problem is that we have a strip mined loop which must 
have a safepoint. That safepoint can only be removed together with the 
OuterStripMinedLoop.

I change my mind and think my webrev.02 is the best fix - make 
SafePoint::Identity to never substitute a strip-mine-loop safepoint.

http://cr.openjdk.java.net/~neliasso/8205107/webrev.02

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