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

Tue Jun 26 09:20:11 UTC 2018

Are you ok with the change?

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

Regards,

Nils

On 2018-06-25 23:08, Nils Eliasson wrote:
>
> On 2018-06-21 19:26, Vladimir Kozlov wrote:
>> 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.
>
> The problem with the test is related to Xcomp. It refuses to inline 
> valueOf beacuse it isn't loaded. The output gets confusing beacuse it 
> delays the inline until the macro-expansion, and then it reports 
> "failed inital checks". Running in normal mode makes it behave as 
> expected.
>
>>
>>
>>>
>>> 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.
>
> This change guarantees that the (OuterStripMinedLoop) safepoint 
> doesn't get removed, even though a very unlikely call is left in the 
> countedloop.
>
> // Nils
>
>>
>> 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.
>>>>>>>>>>
>>>>>>>
>>>>>
>>>
>