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

Nils Eliasson nils.eliasson at oracle.com
Tue Jun 26 16:41:27 UTC 2018 

ok,

Thanks!

// Nils


On 2018-06-26 18:34, Vladimir Kozlov wrote:
> 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.
>>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>>

More information about the hotspot-compiler-dev mailing list