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

[Nils Eliasson]

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.

"(proj_out_or_null(TypeFunc::Parms) == NULL));" seems redundant too me. 
What would that guard against?

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