Re: Fix ppc64le bootstrap comparison failure

Re: Fix ppc64le bootstrap comparison failure

[David Edelsohn]

Jeff,

The ppc64le bootstrap comparison may have been fixed, but I continue
to see the additional testsuite failures on AIX, all of the form:

internal compiler error: in fill_vec_av_set, at sel-sched.c:3712

I rolled back the earlier version of DSE patch and the failures were
resolved, so it definitely seems to be caused by the DSE patch.

Thanks, David

Re: Fix ppc64le bootstrap comparison failure

[Jeff Law]

On 01/14/2017 08:49 AM, David Edelsohn wrote:
> Jeff,
>
> The ppc64le bootstrap comparison may have been fixed, but I continue
> to see the additional testsuite failures on AIX, all of the form:
>
> internal compiler error: in fill_vec_av_set, at sel-sched.c:3712
>
> I rolled back the earlier version of DSE patch and the failures were
> resolved, so it definitely seems to be caused by the DSE patch.
I'm not seeing any new failures on ppc64le-linux.  I've got a big-endian 
ppc system provisioning for testing just in case its an endianness issue.

jeff

Re: Fix ppc64le bootstrap comparison failure

[David Edelsohn]

On Sat, Jan 14, 2017 at 11:46 AM, Jeff Law <law@redhat.com> wrote:
> On 01/14/2017 08:49 AM, David Edelsohn wrote:
>>
>> Jeff,
>>
>> The ppc64le bootstrap comparison may have been fixed, but I continue
>> to see the additional testsuite failures on AIX, all of the form:
>>
>> internal compiler error: in fill_vec_av_set, at sel-sched.c:3712
>>
>> I rolled back the earlier version of DSE patch and the failures were
>> resolved, so it definitely seems to be caused by the DSE patch.
>
> I'm not seeing any new failures on ppc64le-linux.  I've got a big-endian ppc
> system provisioning for testing just in case its an endianness issue.

powerpc64-unknown-linux-gnu (POWER7 BE) at r244463 (after your fix)
shows many failures:

https://gcc.gnu.org/ml/gcc-testresults/2017-01/msg01419.html

Thanks, David

Re: Fix ppc64le bootstrap comparison failure

[Jeff Law]

On 01/14/2017 10:03 AM, David Edelsohn wrote:
> On Sat, Jan 14, 2017 at 11:46 AM, Jeff Law <law@redhat.com> wrote:
>> On 01/14/2017 08:49 AM, David Edelsohn wrote:
>>>
>>> Jeff,
>>>
>>> The ppc64le bootstrap comparison may have been fixed, but I continue
>>> to see the additional testsuite failures on AIX, all of the form:
>>>
>>> internal compiler error: in fill_vec_av_set, at sel-sched.c:3712
>>>
>>> I rolled back the earlier version of DSE patch and the failures were
>>> resolved, so it definitely seems to be caused by the DSE patch.
>>
>> I'm not seeing any new failures on ppc64le-linux.  I've got a big-endian ppc
>> system provisioning for testing just in case its an endianness issue.
>
> powerpc64-unknown-linux-gnu (POWER7 BE) at r244463 (after your fix)
> shows many failures:
>
> https://gcc.gnu.org/ml/gcc-testresults/2017-01/msg01419.html
THanks.  I'll start debugging from that list rather than waiting on my 
own bootstrap & test cycle to speed things up.

jeff

Re: Fix ppc64le bootstrap comparison failure

[Jeff Law]

On 01/14/2017 10:03 AM, David Edelsohn wrote:
> On Sat, Jan 14, 2017 at 11:46 AM, Jeff Law <law@redhat.com> wrote:
>> On 01/14/2017 08:49 AM, David Edelsohn wrote:
>>>
>>> Jeff,
>>>
>>> The ppc64le bootstrap comparison may have been fixed, but I continue
>>> to see the additional testsuite failures on AIX, all of the form:
>>>
>>> internal compiler error: in fill_vec_av_set, at sel-sched.c:3712
>>>
>>> I rolled back the earlier version of DSE patch and the failures were
>>> resolved, so it definitely seems to be caused by the DSE patch.
>>
>> I'm not seeing any new failures on ppc64le-linux.  I've got a big-endian ppc
>> system provisioning for testing just in case its an endianness issue.
>
> powerpc64-unknown-linux-gnu (POWER7 BE) at r244463 (after your fix)
> shows many failures:
>
> https://gcc.gnu.org/ml/gcc-testresults/2017-01/msg01419.html
Found one bug; I'm not sure if it accounts for everything, but it 
probably covers anything involving sel-sched.  Bootstrapping and testing 
now to see if there's other stuff that needs investigation.

jeff

Re: Fix ppc64le bootstrap comparison failure

[Andrew Pinski]

On Fri, Jan 13, 2017 at 10:24 PM, Jeff Law <law@redhat.com> wrote:
>
>
> Given a block with more than one dead store, one of which is the last
> statement in the block, the existence debugging statements can change the
> generated code which is of course bad.
>
> The problem is when I moved the code to delete the dead statement into its
> own function, I passed in the statement rather than the iterator. As a
> result  dse_dom_walker::before_dom_children would not see the iterator in
> the proper state and it would fail to walk all the statements if
> dse_optimize_stmt deleted the last statement in the block.
>
> In a debug build that scenario was much less likely to trigger because of
> the existence of debug statements.
>
> I'm installing this on the trunk now to fix the bootstrap issues.  I'll try
> to construct a testcase Monday.
>
> Bootstrapped and regression tested on x86_64-linux-gnu and ppc64-linux-gnu.
> Installing on the trunk.
>
> Sorry for the breakage.

This fixed the bootstrap issue I was seeing on aarch64-linux-gnu.

Thanks,
Andrew


>
> Jeff
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 3c74b19..d07b1f5 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,14 @@
> +2017-01-13  Jeff Law  <law@redhat.com>
> +
> +       PR tree-optimization/33562
> +       PR tree-optimization/61912
> +       PR tree-optimization/77485
> +       * tree-ssa-dse.c (delete_dead_call): Accept gsi rather than
> +       a statement.
> +       (delete_dead_assignment): Likewise.
> +       (dse_dom_walker::dse_optimize_stmt): Pass in the gsi rather than
> +       statement to delete_dead_call and delete_dead_assignment.
> +
>  2017-01-13  David Malcolm  <dmalcolm@redhat.com>
>
>         PR c/78304
> diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c
> index 20cf3b4..2e6f8ff 100644
> --- a/gcc/tree-ssa-dse.c
> +++ b/gcc/tree-ssa-dse.c
> @@ -600,10 +600,11 @@ private:
>    void dse_optimize_stmt (gimple_stmt_iterator *);
>  };
>
> -/* Delete a dead call STMT, which is mem* call of some kind.  */
> +/* Delete a dead call at GSI, which is mem* call of some kind.  */
>  static void
> -delete_dead_call (gimple *stmt)
> +delete_dead_call (gimple_stmt_iterator *gsi)
>  {
> +  gimple *stmt = gsi_stmt (*gsi);
>    if (dump_file && (dump_flags & TDF_DETAILS))
>      {
>        fprintf (dump_file, "  Deleted dead call: ");
> @@ -612,13 +613,12 @@ delete_dead_call (gimple *stmt)
>      }
>
>    tree lhs = gimple_call_lhs (stmt);
> -  gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
>    if (lhs)
>      {
>        tree ptr = gimple_call_arg (stmt, 0);
>        gimple *new_stmt = gimple_build_assign (lhs, ptr);
>        unlink_stmt_vdef (stmt);
> -      if (gsi_replace (&gsi, new_stmt, true))
> +      if (gsi_replace (gsi, new_stmt, true))
>          bitmap_set_bit (need_eh_cleanup, gimple_bb (stmt)->index);
>      }
>    else
> @@ -627,17 +627,18 @@ delete_dead_call (gimple *stmt)
>        unlink_stmt_vdef (stmt);
>
>        /* Remove the dead store.  */
> -      if (gsi_remove (&gsi, true))
> +      if (gsi_remove (gsi, true))
>         bitmap_set_bit (need_eh_cleanup, gimple_bb (stmt)->index);
>        release_defs (stmt);
>      }
>  }
>
> -/* Delete a dead store STMT, which is a gimple assignment. */
> +/* Delete a dead store at GSI, which is a gimple assignment. */
>
>  static void
> -delete_dead_assignment (gimple *stmt)
> +delete_dead_assignment (gimple_stmt_iterator *gsi)
>  {
> +  gimple *stmt = gsi_stmt (*gsi);
>    if (dump_file && (dump_flags & TDF_DETAILS))
>      {
>        fprintf (dump_file, "  Deleted dead store: ");
> @@ -649,9 +650,8 @@ delete_dead_assignment (gimple *stmt)
>    unlink_stmt_vdef (stmt);
>
>    /* Remove the dead store.  */
> -  gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
>    basic_block bb = gimple_bb (stmt);
> -  if (gsi_remove (&gsi, true))
> +  if (gsi_remove (gsi, true))
>      bitmap_set_bit (need_eh_cleanup, bb->index);
>
>    /* And release any SSA_NAMEs set in this statement back to the
> @@ -717,7 +717,7 @@ dse_dom_walker::dse_optimize_stmt (gimple_stmt_iterator
> *gsi)
>                 }
>
>               if (store_status == DSE_STORE_DEAD)
> -               delete_dead_call (stmt);
> +               delete_dead_call (gsi);
>               return;
>             }
>
> @@ -760,7 +760,7 @@ dse_dom_walker::dse_optimize_stmt (gimple_stmt_iterator
> *gsi)
>           && !gimple_clobber_p (use_stmt))
>         return;
>
> -      delete_dead_assignment (stmt);
> +      delete_dead_assignment (gsi);
>      }
>  }
>
>

Fix ppc64le bootstrap comparison failure

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]



Given a block with more than one dead store, one of which is the last 
statement in the block, the existence debugging statements can change 
the generated code which is of course bad.

The problem is when I moved the code to delete the dead statement into 
its own function, I passed in the statement rather than the iterator. 
As a result  dse_dom_walker::before_dom_children would not see the 
iterator in the proper state and it would fail to walk all the 
statements if dse_optimize_stmt deleted the last statement in the block.

In a debug build that scenario was much less likely to trigger because 
of the existence of debug statements.

I'm installing this on the trunk now to fix the bootstrap issues.  I'll 
try to construct a testcase Monday.

Bootstrapped and regression tested on x86_64-linux-gnu and 
ppc64-linux-gnu.  Installing on the trunk.

Sorry for the breakage.

Jeff

[-- Attachment #2: P --]
[-- Type: text/plain, Size: 3145 bytes --]

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 3c74b19..d07b1f5 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,14 @@
+2017-01-13  Jeff Law  <law@redhat.com>
+
+	PR tree-optimization/33562
+	PR tree-optimization/61912
+	PR tree-optimization/77485
+	* tree-ssa-dse.c (delete_dead_call): Accept gsi rather than
+	a statement.
+	(delete_dead_assignment): Likewise.
+	(dse_dom_walker::dse_optimize_stmt): Pass in the gsi rather than
+	statement to delete_dead_call and delete_dead_assignment.
+
 2017-01-13  David Malcolm  <dmalcolm@redhat.com>
 
 	PR c/78304
diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c
index 20cf3b4..2e6f8ff 100644
--- a/gcc/tree-ssa-dse.c
+++ b/gcc/tree-ssa-dse.c
@@ -600,10 +600,11 @@ private:
   void dse_optimize_stmt (gimple_stmt_iterator *);
 };
 
-/* Delete a dead call STMT, which is mem* call of some kind.  */
+/* Delete a dead call at GSI, which is mem* call of some kind.  */
 static void
-delete_dead_call (gimple *stmt)
+delete_dead_call (gimple_stmt_iterator *gsi)
 {
+  gimple *stmt = gsi_stmt (*gsi);
   if (dump_file && (dump_flags & TDF_DETAILS))
     {
       fprintf (dump_file, "  Deleted dead call: ");
@@ -612,13 +613,12 @@ delete_dead_call (gimple *stmt)
     }
 
   tree lhs = gimple_call_lhs (stmt);
-  gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
   if (lhs)
     {
       tree ptr = gimple_call_arg (stmt, 0);
       gimple *new_stmt = gimple_build_assign (lhs, ptr);
       unlink_stmt_vdef (stmt);
-      if (gsi_replace (&gsi, new_stmt, true))
+      if (gsi_replace (gsi, new_stmt, true))
         bitmap_set_bit (need_eh_cleanup, gimple_bb (stmt)->index);
     }
   else
@@ -627,17 +627,18 @@ delete_dead_call (gimple *stmt)
       unlink_stmt_vdef (stmt);
 
       /* Remove the dead store.  */
-      if (gsi_remove (&gsi, true))
+      if (gsi_remove (gsi, true))
 	bitmap_set_bit (need_eh_cleanup, gimple_bb (stmt)->index);
       release_defs (stmt);
     }
 }
 
-/* Delete a dead store STMT, which is a gimple assignment. */
+/* Delete a dead store at GSI, which is a gimple assignment. */
 
 static void
-delete_dead_assignment (gimple *stmt)
+delete_dead_assignment (gimple_stmt_iterator *gsi)
 {
+  gimple *stmt = gsi_stmt (*gsi);
   if (dump_file && (dump_flags & TDF_DETAILS))
     {
       fprintf (dump_file, "  Deleted dead store: ");
@@ -649,9 +650,8 @@ delete_dead_assignment (gimple *stmt)
   unlink_stmt_vdef (stmt);
 
   /* Remove the dead store.  */
-  gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
   basic_block bb = gimple_bb (stmt);
-  if (gsi_remove (&gsi, true))
+  if (gsi_remove (gsi, true))
     bitmap_set_bit (need_eh_cleanup, bb->index);
 
   /* And release any SSA_NAMEs set in this statement back to the
@@ -717,7 +717,7 @@ dse_dom_walker::dse_optimize_stmt (gimple_stmt_iterator *gsi)
 		}
 
 	      if (store_status == DSE_STORE_DEAD)
-		delete_dead_call (stmt);
+		delete_dead_call (gsi);
 	      return;
 	    }
 
@@ -760,7 +760,7 @@ dse_dom_walker::dse_optimize_stmt (gimple_stmt_iterator *gsi)
 	  && !gimple_clobber_p (use_stmt))
 	return;
 
-      delete_dead_assignment (stmt);
+      delete_dead_assignment (gsi);
     }
 }