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

[Vladimir Kozlov]

On 6/21/18 7:54 AM, Nils Eliasson wrote:
> 
> 
> 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.

It is not wrong to have a Call node and Safepoint in a loop. It is just 
not optimal - that is all.
I remember that it could be cases when even if boxing call is marked as 
not escaping (or scalar replaceable) it may not be eliminated because of 
it has still have some uses left which were not removed for different 
reasons.
I agree that it is not easy problem.

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

Okay, but please extend your comment to explain when we can have a 
Boxing Call node which may be late removed in Macro extension phase 
leaving loop without safepoint.

Thanks,
Vladimir

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