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

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Nils Eliasson
Hi,

This patch fixes a problem where we eliminate the safepoint of a strip
mined loop, and later hit an assert.

I add a check in SafepointNode::Identity to not remove the safepoint
node if it belongs to a OuterStripMinedLoopEndNode. If the loop is
eliminated, the safepoint will be removed together with the
OuterStripMinedLoop and OuterStripMinedLoopEnd, maintaining a consistent
shape until it is gone.

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

https://bugs.openjdk.java.net/browse/JDK-8205107

Regards,
Nils Eliasson
Reply | Threaded
Open this post in threaded view
|

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

Roland Westrelin-3

Hi Nils,

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

Looks like the webrev for the wrong bug.

Roland.
Reply | Threaded
Open this post in threaded view
|

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

Nils Eliasson

Hi,

Here is the correct one: http://cr.openjdk.java.net/~neliasso/8205107/webrev.02/

Thanks,

Nils


On 2018-06-15 14:53, Roland Westrelin wrote:
Hi Nils,

http://cr.openjdk.java.net/~neliasso/8205107/webrev.01/
Looks like the webrev for the wrong bug.

Roland.

Reply | Threaded
Open this post in threaded view
|

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

Roland Westrelin-3

> Here is the correct one:
> http://cr.openjdk.java.net/~neliasso/8205107/webrev.02/

Looks ok to me.

Roland.
Reply | Threaded
Open this post in threaded view
|

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

Nils Eliasson
Thank you!

// N


On 2018-06-15 15:12, Roland Westrelin wrote:
>> Here is the correct one:
>> http://cr.openjdk.java.net/~neliasso/8205107/webrev.02/
> Looks ok to me.
>
> Roland.

Reply | Threaded
Open this post in threaded view
|

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

Vladimir Kozlov
In reply to this post by Nils Eliasson
Hi Nils,

I don't see link to testing in bug report.

Roland, why we need to keep Safepoint in Outer loop if we have call?

Why we even do strip mining if we have a call in a loop?

Thanks,
Vladimir

On 6/15/18 5:49 AM, Nils Eliasson wrote:

> Hi,
>
> Here is the correct one:
> http://cr.openjdk.java.net/~neliasso/8205107/webrev.02/
>
> Thanks,
>
> Nils
>
>
> On 2018-06-15 14:53, Roland Westrelin wrote:
>> Hi Nils,
>>
>>> http://cr.openjdk.java.net/~neliasso/8205107/webrev.01/
>> Looks like the webrev for the wrong bug.
>>
>> Roland.
>
Reply | Threaded
Open this post in threaded view
|

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

Roland Westrelin-3

> 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.
Reply | Threaded
Open this post in threaded view
|

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

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

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.

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

Reply | Threaded
Open this post in threaded view
|

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

Vladimir Kozlov
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); }

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.
>
Reply | Threaded
Open this post in threaded view
|

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

Vladimir Kozlov
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.
>>
Reply | Threaded
Open this post in threaded view
|

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

Reply | Threaded
Open this post in threaded view
|

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

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

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.
>>>>
>
Reply | Threaded
Open this post in threaded view
|

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

Nils Eliasson


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.

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

Reply | Threaded
Open this post in threaded view
|

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

Vladimir Kozlov
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?

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.
>>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

Nils Eliasson


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.

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

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

Reply | Threaded
Open this post in threaded view
|

Re: 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.
>>>>>>>>
>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

Nils Eliasson

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

Reply | Threaded
Open this post in threaded view
|

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

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

Reply | Threaded
Open this post in threaded view
|

Re: 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.
>>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

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

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