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

[Vladimir Kozlov]

It is old webrev. Comment did not change.

Vladimir

On 6/26/18 2:20 AM, Nils Eliasson wrote:
> 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.
>>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
>