diff mbox series

Fix pessimization in EH cleanup pass

Message ID 6993380.JPCJVM3AQ6@fomalhaut
State New
Headers show
Series Fix pessimization in EH cleanup pass | expand

Commit Message

Eric Botcazou Sept. 15, 2020, 8:36 a.m. UTC 
Hi,

the cleanup_all_empty_eh function was originally doing a post-order traversal 
of the EH region tree to optimize it:

/* Do a post-order traversal of the EH region tree.  Examine each
   post_landing_pad block and see if we can eliminate it as empty.  */

That's sensible since the worker function cleanup_empty_eh punts if the number 
of outgoing edges from landing pads is not 0 or 1:

  /* There can be zero or one edges out of BB.  This is the quickest test.  */
  switch (EDGE_COUNT (bb->succs))
    {
    case 0:
      e_out = NULL;
      break;
    case 1:
      e_out = single_succ_edge (bb);
      break;
    default:
      return false;
    }

This was recently changed to use another order and this trivially breaks 
testcases with nested regions like the attached one.  So the attached patch 
restores the post-order traversal and it also contains a small tweak to the 
line debug info to avoid a problematic inheritance for coverage measurement.

Tested on x86_64-suse-linux, OK for the mainline?


2020-09-15  Eric Botcazou  <ebotcazou@adacore.com>
            Pierre-Marie Derodat  <derodat@adacore.com>

	* tree-eh.c (lower_try_finally_dup_block): Backward propagate slocs 
	to stack restore builtin calls.
	(cleanup_all_empty_eh): Do again a post-order traversal of the EH
	region tree.


2020-09-15  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/concat4.adb: New test.

Comments

Jakub Jelinek Sept. 15, 2020, 8:48 a.m. UTC | #1 
On Tue, Sep 15, 2020 at 10:36:20AM +0200, Eric Botcazou wrote:
> This was recently changed to use another order and this trivially breaks 
> testcases with nested regions like the attached one.  So the attached patch 
> restores the post-order traversal and it also contains a small tweak to the 
> line debug info to avoid a problematic inheritance for coverage measurement.

So it breaks PR93199 again?

	Jakub
Eric Botcazou Sept. 15, 2020, 11:27 a.m. UTC | #2 
> So it breaks PR93199 again?

Indeed, although there is no regression in the testsuite AFAICS.  I guess that 
we can do the new walk before and not instead of the post-order traversal.

Revised patch attached, same ChangeLog.
Richard Biener Sept. 15, 2020, 11:47 a.m. UTC | #3 
On Tue, Sep 15, 2020 at 1:29 PM Eric Botcazou <botcazou@adacore.com> wrote:
>
> > So it breaks PR93199 again?
>
> Indeed, although there is no regression in the testsuite AFAICS.

Yeah, too big of a testcase ...

>  I guess that
> we can do the new walk before and not instead of the post-order traversal.
>
> Revised patch attached, same ChangeLog.

If that avoids the compile-time regression it's fine.

Thanks,
Richard.

> --
> Eric Botcazou
diff mbox series

Patch

diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c
index 4246dca8806..ed32f80220d 100644
--- a/gcc/tree-eh.c
+++ b/gcc/tree-eh.c
@@ -899,23 +899,26 @@  lower_try_finally_dup_block (gimple_seq seq, struct leh_state *outer_state,
   gtry *region = NULL;
   gimple_seq new_seq;
   gimple_stmt_iterator gsi;
+  location_t last_loc = UNKNOWN_LOCATION;
 
   new_seq = copy_gimple_seq_and_replace_locals (seq);
 
-  for (gsi = gsi_start (new_seq); !gsi_end_p (gsi); gsi_next (&gsi))
+  for (gsi = gsi_last (new_seq); !gsi_end_p (gsi); gsi_prev (&gsi))
     {
       gimple *stmt = gsi_stmt (gsi);
       /* We duplicate __builtin_stack_restore at -O0 in the hope of eliminating
-	 it on the EH paths.  When it is not eliminated, make it transparent in
-	 the debug info.  */
+	 it on the EH paths.  When it is not eliminated, give it the next
+	 location in the sequence or make it transparent in the debug info.  */
       if (gimple_call_builtin_p (stmt, BUILT_IN_STACK_RESTORE))
-	gimple_set_location (stmt, UNKNOWN_LOCATION);
+	gimple_set_location (stmt, last_loc);
       else if (LOCATION_LOCUS (gimple_location (stmt)) == UNKNOWN_LOCATION)
 	{
 	  tree block = gimple_block (stmt);
 	  gimple_set_location (stmt, loc);
 	  gimple_set_block (stmt, block);
 	}
+      else
+	last_loc = gimple_location (stmt);
     }
 
   if (outer_state->tf)
@@ -4751,15 +4754,9 @@  cleanup_all_empty_eh (void)
   eh_landing_pad lp;
   int i;
 
-  /* Ideally we'd walk the region tree and process LPs inner to outer
-     to avoid quadraticness in EH redirection.  Walking the LP array
-     in reverse seems to be an approximation of that.  */
-  for (i = vec_safe_length (cfun->eh->lp_array) - 1; i >= 1; --i)
-    {
-      lp = (*cfun->eh->lp_array)[i];
-      if (lp)
-	changed |= cleanup_empty_eh (lp);
-    }
+  for (i = 1; vec_safe_iterate (cfun->eh->lp_array, i, &lp); ++i)
+    if (lp)
+      changed |= cleanup_empty_eh (lp);
 
   return changed;
 }