Quantcast

RFR: 8144484: assert(no_dead_loop) failed: dead loop detected

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR: 8144484: assert(no_dead_loop) failed: dead loop detected

Rahul Raghavan
Hi,

Please review following fix patch for JDK-8144484.

<webrev.00> -  http://cr.openjdk.java.net/~rraghavan/8144484/webrev.00/

Please check the details in <jbs> https://bugs.openjdk.java.net/browse/JDK-8144484


1. Found the root cause of the issue as strong dead loop checks not done in PhiNode::Ideal()
  before splitting Phi through memory merges,
  before new MergeMemNodes creation, which is not dead_loop_safe.


2. Above webrev.00 fix proposal in PhiNode::Ideal() [src/share/vm/opto/cfgnode.cpp]
Related code extracts for reference -

- - - - - - - - - - - - - - - - - - - - - - - - -
// Return a node which is more "ideal" than the current node. Must preserve
// the CFG, but we can still strip out dead paths.
Node *PhiNode::Ideal(PhaseGVN *phase, bool can_reshape) {
  ...........
  ...........
  // Split phis through memory merges, so that the memory merges will go away.
  // Piggy-back this transformation on the search for a unique input....
  // It will be as if the merged memory is the unique value of the phi.
  // (Do not attempt this optimization unless parsing is complete.
  // It would make the parser's memory-merge logic sick.)
  // (MergeMemNode is not dead_loop_safe - need to check for dead loop.)
  if (progress == NULL && can_reshape && type() == Type::MEMORY) {
    ...........
    ...........
       // We know that at least one MergeMem->base_memory() == this
        // (saw_self == true). If all other inputs also references this phi
        // (directly or through data nodes) - it is dead loop.
         bool saw_safe_input = false;
         for (uint j = 1; j < req(); ++j) {
           Node *n = in(j);
           if (n->is_MergeMem() && n->as_MergeMem()->base_memory() == this) {
             continue; // skip known cases
           }
+        // TOP inputs should not be counted as safe inputs because if the
+        // Phi references itself through all other inputs then splitting the
+        // Phi through memory merges would create dead loop at later stage..
+        if (n == top) {
+        continue;
+        }
           if (!is_unsafe_data_reference(n)) {
             saw_safe_input = true; // found safe input
             break;
          }
        }
        if (!saw_safe_input)
          return top; // all inputs reference back to this phi - dead loop

        // Phi(...MergeMem(m0, m1:AT1, m2:AT2)...) into
        // MergeMem(Phi(...m0...), Phi:AT1(...m1...), Phi:AT2(...m2...))
   ...................
- - - - - - - - - - - - - - - - - - - - - - - - -


3. For the failing test case, following is the related input graph nodes dump in PhiNode::Ideal() before checking for possible dead loop
    (before confirming saw_safe_input)

      2808 Region === 2808 1 2980 2804 [[ 2808 3002 2987 2985 ]] ..........
          2980 IfFalse === 2978 [[ 2808 ]] ..........
          2804 IfTrue === 2803 [[ 2808 ]] ..........
      2985 Phi === 2808 1 6044 3011 [[ 3020 3003 3026 3011 2860 2785 2769 2821 2833 2943 2903 ]] ..........
          6044 MergeMem === _ 1 3011 1 1 1 6045 [[ 6050 6048 2985 ]] ..........
          3011 MergeMem === _ 1 2985 1 1 1 2987 [[ 2987 2985 6045 6044 ]] ..........

Wrongly considering TOP inputs as safe inputs results in dead loop at later stage.
Without the fix, related graph nodes dumped during failing case -
   3011 MergeMem === _ 1 6058 1 1 1 2987 [[ 2987 6058 6045 6044 ]] ...
   6058 MergeMem === _ 1 3011 1 1 1 6059 [[ 2903 2943 2833 2821 2769 2785 2860 3011 3026 3003 3020 ]] ...
The dead loop causing the crash -- [6058] > [3011] > [6058] > [3011]


4. So proposed webrev.00 fix in PhiNode::Ideal() is to ignore the TOP inputs before checking the is_unsafe_data_reference(n),
  so that saw_safe_input is set to the correct value.
TOP inputs should be ignored else if there is an actual dead loop
  .i.e. if the Phi references to itself through all other inputs
  then splitting the Phi through memory merges would create dead loop at later stage
  because Phi with TOP inputs will go away exposing the dead loop.


Confirmed the fix with 8144484 test and JPRT (-testset hotspot). RBT Pre-integration testing in progress.


Thanks,
Rahul
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8144484: assert(no_dead_loop) failed: dead loop detected

Tobias Hartmann-2
Hi Rahul,

good work and nice evaluation!

On 31.01.2017 17:51, Rahul Raghavan wrote:
<webrev.00> -  http://cr.openjdk.java.net/~rraghavan/8144484/webrev.00/

As we discussed off-thread, this looks good to me but why did you remove the assert in line 2100?

Thanks,
Tobias
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: RFR: 8144484: assert(no_dead_loop) failed: dead loop detected

Rahul Raghavan
Hi,

> why did you remove the assert in line 2100?

Sorry that was by mistake.
Removing the assert is not part of the fix.

Please see updated <webrev.01> -
http://cr.openjdk.java.net/~rraghavan/8144484/webrev.01/
Sorry for the noise.

Thank you Tobias for help and review.

-Rahul

> -----Original Message-----
> From: Tobias Hartmann
> Sent: Tuesday, January 31, 2017 10:24 PM
> To: Rahul Raghavan; [hidden email]
> Subject: Re: RFR: 8144484: assert(no_dead_loop) failed: dead loop detected
>
> Hi Rahul,
>
> good work and nice evaluation!
>
> On 31.01.2017 17:51, Rahul Raghavan wrote:
> <webrev.00> -  http://cr.openjdk.java.net/~rraghavan/8144484/webrev.00/
>
> As we discussed off-thread, this looks good to me but why did you remove the assert in line 2100?
>
> Thanks,
> Tobias


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8144484: assert(no_dead_loop) failed: dead loop detected

Tobias Hartmann-2
Hi Rahul,

On 31.01.2017 19:55, Rahul Raghavan wrote:
> Please see updated <webrev.01> -
> http://cr.openjdk.java.net/~rraghavan/8144484/webrev.01/

Looks good to me!

Best regards,
Tobias
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: RFR: 8144484: assert(no_dead_loop) failed: dead loop detected

Rahul Raghavan
Thank you Tobias.

> -----Original Message-----
> From: Tobias Hartmann
> Sent: Wednesday, February 01, 2017 1:09 PM
> To: Rahul Raghavan; [hidden email]
> Subject: Re: RFR: 8144484: assert(no_dead_loop) failed: dead loop detected
>
> Hi Rahul,
>
> On 31.01.2017 19:55, Rahul Raghavan wrote:
> > Please see updated <webrev.01> -
> > http://cr.openjdk.java.net/~rraghavan/8144484/webrev.01/
>
> Looks good to me!
>
> Best regards,
> Tobias
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8144484: assert(no_dead_loop) failed: dead loop detected

Vladimir Kozlov
In reply to this post by Tobias Hartmann-2
Sorry, I don't like this fix.

Based on your evaluation one control path is dead (top) and it will be optimized out later. It was unfortunate that Phi
node was processed first. Change graph (split through MergeMem) is dangerous in this state of graph.

I would suggest to bailout early:

1886   if (progress == NULL && can_reshape && type() == Type::MEMORY) {
1887     // see if this phi should be sliced
1888     uint merge_width = 0;
1889     bool saw_self = false;
1890     for( uint i=1; i<req(); ++i ) {// For all paths in
1891       Node *ii = in(i);

+          // TOP inputs should not be counted as safe inputs because if the
+          // Phi references itself through all other inputs then splitting the
+          // Phi through memory merges would create dead loop at later stage.
+          if (ii == top) {
+            return top; // Delay optimization until graph is cleaned.
+          }

1892       if (ii->is_MergeMem()) {
1893         MergeMemNode* n = ii->as_MergeMem();

Thanks,
Vladimir

On 1/31/17 11:38 PM, Tobias Hartmann wrote:

> Hi Rahul,
>
> On 31.01.2017 19:55, Rahul Raghavan wrote:
>> Please see updated <webrev.01> -
>> http://cr.openjdk.java.net/~rraghavan/8144484/webrev.01/
>
> Looks good to me!
>
> Best regards,
> Tobias
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8144484: assert(no_dead_loop) failed: dead loop detected

Vladimir Kozlov
On 2/1/17 12:19 AM, Vladimir Kozlov wrote:

> Sorry, I don't like this fix.
>
> Based on your evaluation one control path is dead (top) and it will be optimized out later. It was unfortunate that Phi
> node was processed first. Change graph (split through MergeMem) is dangerous in this state of graph.
>
> I would suggest to bailout early:
>
> 1886   if (progress == NULL && can_reshape && type() == Type::MEMORY) {
> 1887     // see if this phi should be sliced
> 1888     uint merge_width = 0;
> 1889     bool saw_self = false;
> 1890     for( uint i=1; i<req(); ++i ) {// For all paths in
> 1891       Node *ii = in(i);
>
> +          // TOP inputs should not be counted as safe inputs because if the
> +          // Phi references itself through all other inputs then splitting the
> +          // Phi through memory merges would create dead loop at later stage.
> +          if (ii == top) {
> +            return top; // Delay optimization until graph is cleaned.

Sorry, wrong return value. Should be return NULL;

> +          }
>
> 1892       if (ii->is_MergeMem()) {
> 1893         MergeMemNode* n = ii->as_MergeMem();
>
> Thanks,
> Vladimir
>
> On 1/31/17 11:38 PM, Tobias Hartmann wrote:
>> Hi Rahul,
>>
>> On 31.01.2017 19:55, Rahul Raghavan wrote:
>>> Please see updated <webrev.01> -
>>> http://cr.openjdk.java.net/~rraghavan/8144484/webrev.01/
>>
>> Looks good to me!
>>
>> Best regards,
>> Tobias
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: RFR: 8144484: assert(no_dead_loop) failed: dead loop detected

Rahul Raghavan
Hi,

Thank you Vladimir for the review comments.
Understood your point and agree that for the reported scenario
bailing out early and delaying the optimization should be the fix.

Please review updated <webrev.02> - http://cr.openjdk.java.net/~rraghavan/8144484/webrev.02/

Confirmed no issues with testing
(8144484 test, jprt -testset hotspot; so far no issues with RBT Pre-integration testing in progress )

Thanks,
Rahul

> -----Original Message-----
> From: Vladimir Kozlov
> Sent: Wednesday, February 01, 2017 2:32 PM
> To: [hidden email]
> Subject: Re: RFR: 8144484: assert(no_dead_loop) failed: dead loop detected
>
> On 2/1/17 12:19 AM, Vladimir Kozlov wrote:
> > Sorry, I don't like this fix.
> >
> > Based on your evaluation one control path is dead (top) and it will be optimized out later. It was unfortunate that Phi
> > node was processed first. Change graph (split through MergeMem) is dangerous in this state of graph.
> >
> > I would suggest to bailout early:
> >
> > 1886   if (progress == NULL && can_reshape && type() == Type::MEMORY) {
> > 1887     // see if this phi should be sliced
> > 1888     uint merge_width = 0;
> > 1889     bool saw_self = false;
> > 1890     for( uint i=1; i<req(); ++i ) {// For all paths in
> > 1891       Node *ii = in(i);
> >
> > +          // TOP inputs should not be counted as safe inputs because if the
> > +          // Phi references itself through all other inputs then splitting the
> > +          // Phi through memory merges would create dead loop at later stage.
> > +          if (ii == top) {
> > +            return top; // Delay optimization until graph is cleaned.
>
> Sorry, wrong return value. Should be return NULL;
>
> > +          }
> >
> > 1892       if (ii->is_MergeMem()) {
> > 1893         MergeMemNode* n = ii->as_MergeMem();
> >
> > Thanks,
> > Vladimir
> >
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8144484: assert(no_dead_loop) failed: dead loop detected

Vladimir Kozlov
On 2/2/17 8:59 AM, Rahul Raghavan wrote:
> Hi,
>
> Thank you Vladimir for the review comments.
> Understood your point and agree that for the reported scenario
> bailing out early and delaying the optimization should be the fix.
>
> Please review updated <webrev.02> - http://cr.openjdk.java.net/~rraghavan/8144484/webrev.02/

Good.

Thanks,
Vladimir

>
> Confirmed no issues with testing
> (8144484 test, jprt -testset hotspot; so far no issues with RBT Pre-integration testing in progress )
>
> Thanks,
> Rahul
>
>> -----Original Message-----
>> From: Vladimir Kozlov
>> Sent: Wednesday, February 01, 2017 2:32 PM
>> To: [hidden email]
>> Subject: Re: RFR: 8144484: assert(no_dead_loop) failed: dead loop detected
>>
>> On 2/1/17 12:19 AM, Vladimir Kozlov wrote:
>>> Sorry, I don't like this fix.
>>>
>>> Based on your evaluation one control path is dead (top) and it will be optimized out later. It was unfortunate that Phi
>>> node was processed first. Change graph (split through MergeMem) is dangerous in this state of graph.
>>>
>>> I would suggest to bailout early:
>>>
>>> 1886   if (progress == NULL && can_reshape && type() == Type::MEMORY) {
>>> 1887     // see if this phi should be sliced
>>> 1888     uint merge_width = 0;
>>> 1889     bool saw_self = false;
>>> 1890     for( uint i=1; i<req(); ++i ) {// For all paths in
>>> 1891       Node *ii = in(i);
>>>
>>> +          // TOP inputs should not be counted as safe inputs because if the
>>> +          // Phi references itself through all other inputs then splitting the
>>> +          // Phi through memory merges would create dead loop at later stage.
>>> +          if (ii == top) {
>>> +            return top; // Delay optimization until graph is cleaned.
>>
>> Sorry, wrong return value. Should be return NULL;
>>
>>> +          }
>>>
>>> 1892       if (ii->is_MergeMem()) {
>>> 1893         MergeMemNode* n = ii->as_MergeMem();
>>>
>>> Thanks,
>>> Vladimir
>>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8144484: assert(no_dead_loop) failed: dead loop detected

Tobias Hartmann-2
In reply to this post by Rahul Raghavan
Hi Rahul,

On 02.02.2017 17:59, Rahul Raghavan wrote:
> Please review updated <webrev.02> - http://cr.openjdk.java.net/~rraghavan/8144484/webrev.02/

Looks good to me!

Thanks,
Tobias
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: RFR: 8144484: assert(no_dead_loop) failed: dead loop detected

Rahul Raghavan
Thank you for the review Tobias, Vladimir.

> -----Original Message-----
> From: Tobias Hartmann
> Sent: Friday, February 03, 2017 12:43 PM
> To: Rahul Raghavan; Vladimir Kozlov; [hidden email]
> Subject: Re: RFR: 8144484: assert(no_dead_loop) failed: dead loop detected
>
> Hi Rahul,
>
> On 02.02.2017 17:59, Rahul Raghavan wrote:
> > Please review updated <webrev.02> - http://cr.openjdk.java.net/~rraghavan/8144484/webrev.02/
>
> Looks good to me!
>
> Thanks,
> Tobias

On 2/2/17 8:59 AM, Rahul Raghavan wrote:
> Hi,
>
> Thank you Vladimir for the review comments.
> Understood your point and agree that for the reported scenario
> bailing out early and delaying the optimization should be the fix.
>
> Please review updated <webrev.02> - http://cr.openjdk.java.net/~rraghavan/8144484/webrev.02/

Good.

Thanks,
Vladimir
Loading...