[vtab (and trunk?)] fix fallout from SSA-coalescing user variables

[vtab (and trunk?)] fix fallout from SSA-coalescing user variables

[Alexandre Oliva]

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

After installing the patch that enables SSA coalescing of all
variables, I've experienced some compilation errors in Ada, having to
do with EH tables.

As it turns out, the compiler was not ready for such coalescing of
gimple registers.  It created situations in which, because of the
abnormal setjmp/longjmp edges used for EH in the (stage1?) Ada
compiler, we'd get variables that were gimple registers and that had
multiple live versions simultaneously renamed at the time of
inlining.  This should never happen.

This patch arranges for us to note the correct variable version to use
at the new edges for such gimple registers, instead of incorrectly
relying on renaming to do that.

:ADDPATCH vtab/inline:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-debug-coalesce-inline-fallout.patch --]
[-- Type: text/x-patch, Size: 6982 bytes --]

Index: gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	* tree-cfg.c (verify_stmts): Check for missing PHI defs.
	* tree-inline.c (update_ssa_across_eh_edges): Renamed to...
	(update_ssa_across_abnormal_edges): ... this.  Set slots in the
	return PHI node.
	(copy_edges_for_bb): Handle nonlocal label edges.
	(make_nonlocal_label_edges): Deleted.
	(optimize_inline_calls): Don't call it.

Index: gcc/tree-inline.c
===================================================================
--- gcc/tree-inline.c.orig	2007-09-28 00:03:12.000000000 -0300
+++ gcc/tree-inline.c	2007-09-28 00:03:16.000000000 -0300
@@ -1018,7 +1018,8 @@ copy_bb (copy_body_data *id, basic_block
    we might want to change way build CFG pre-inlining to include
    all the possible edges then.  */
 static void
-update_ssa_across_eh_edges (basic_block bb)
+update_ssa_across_abnormal_edges (basic_block bb, basic_block ret_bb,
+				  bool can_throw, bool nonlocal_goto)
 {
   edge e;
   edge_iterator ei;
@@ -1029,13 +1030,35 @@ update_ssa_across_eh_edges (basic_block 
       {
 	tree phi;
 
-	gcc_assert (e->flags & EDGE_EH);
+	gcc_assert (e->flags & EDGE_ABNORMAL);
+	if (!nonlocal_goto)
+	  gcc_assert (e->flags & EDGE_EH);
+	if (!can_throw)
+	  gcc_assert (!(e->flags & EDGE_EH));
 	for (phi = phi_nodes (e->dest); phi; phi = PHI_CHAIN (phi))
 	  {
+	    edge re;
+
+	    /* There shouldn't be any PHI nodes in the ENTRY_BLOCK.  */
+	    gcc_assert (!e->dest->aux);
+
 	    gcc_assert (SSA_NAME_OCCURS_IN_ABNORMAL_PHI
 			(PHI_RESULT (phi)));
-	    mark_sym_for_renaming
-	      (SSA_NAME_VAR (PHI_RESULT (phi)));
+
+	    if (!is_gimple_reg (PHI_RESULT (phi)))
+	      {
+		mark_sym_for_renaming
+		  (SSA_NAME_VAR (PHI_RESULT (phi)));
+		continue;
+	      }
+
+	    re = find_edge (ret_bb, e->dest);
+	    gcc_assert (re);
+	    gcc_assert ((re->flags & (EDGE_EH | EDGE_ABNORMAL))
+			== (e->flags & (EDGE_EH | EDGE_ABNORMAL)));
+
+	    SET_USE (PHI_ARG_DEF_PTR_FROM_EDGE (phi, e),
+		     USE_FROM_PTR (PHI_ARG_DEF_PTR_FROM_EDGE (phi, re)));
 	  }
       }
 }
@@ -1044,7 +1067,7 @@ update_ssa_across_eh_edges (basic_block 
    accordingly.  Edges will be taken care of later.  Assume aux
    pointers to point to the copies of each BB.  */
 static void
-copy_edges_for_bb (basic_block bb, int count_scale)
+copy_edges_for_bb (basic_block bb, int count_scale, basic_block ret_bb)
 {
   basic_block new_bb = (basic_block) bb->aux;
   edge_iterator ei;
@@ -1076,6 +1099,7 @@ copy_edges_for_bb (basic_block bb, int c
   for (bsi = bsi_start (new_bb); !bsi_end_p (bsi);)
     {
       tree copy_stmt;
+      bool can_throw, nonlocal_goto;
 
       copy_stmt = bsi_stmt (bsi);
       update_stmt (copy_stmt);
@@ -1096,7 +1120,10 @@ copy_edges_for_bb (basic_block bb, int c
          into a COMPONENT_REF which doesn't.  If the copy
          can throw, the original could also throw.  */
 
-      if (tree_can_throw_internal (copy_stmt))
+      can_throw = tree_can_throw_internal (copy_stmt);
+      nonlocal_goto = tree_can_make_abnormal_goto (copy_stmt);
+
+      if (can_throw || nonlocal_goto)
 	{
 	  if (!bsi_end_p (bsi))
 	    /* Note that bb's predecessor edges aren't necessarily
@@ -1108,12 +1135,18 @@ copy_edges_for_bb (basic_block bb, int c
 	      new_bb->aux = e->src->aux;
 	      bsi = bsi_start (new_bb);
 	    }
+	}
 
-           make_eh_edges (copy_stmt);
+      if (can_throw)
+	make_eh_edges (copy_stmt);
 
-	   if (gimple_in_ssa_p (cfun))
-	     update_ssa_across_eh_edges (bb_for_stmt (copy_stmt));
-	}
+      if (nonlocal_goto)
+	make_abnormal_goto_edges (bb_for_stmt (copy_stmt), true);
+
+      if ((can_throw || nonlocal_goto)
+	  && gimple_in_ssa_p (cfun))
+	update_ssa_across_abnormal_edges (bb_for_stmt (copy_stmt), ret_bb,
+					  can_throw, nonlocal_goto);
     }
 }
 
@@ -1285,7 +1318,7 @@ copy_cfg_body (copy_body_data * id, gcov
   last = last_basic_block;
   /* Now that we've duplicated the blocks, duplicate their edges.  */
   FOR_ALL_BB_FN (bb, cfun_to_copy)
-    copy_edges_for_bb (bb, count_scale);
+    copy_edges_for_bb (bb, count_scale, exit_block_map);
   if (gimple_in_ssa_p (cfun))
     FOR_ALL_BB_FN (bb, cfun_to_copy)
       copy_phis_for_bb (bb, id);
@@ -2803,60 +2836,6 @@ has_abnormal_outgoing_edge_p (basic_bloc
   return false;
 }
 
-/* When a block from the inlined function contains a call with side-effects
-   in the middle gets inlined in a function with non-locals labels, the call
-   becomes a potential non-local goto so we need to add appropriate edge.  */
-
-static void
-make_nonlocal_label_edges (void)
-{
-  block_stmt_iterator bsi;
-  basic_block bb;
-
-  FOR_EACH_BB (bb)
-    {
-      for (bsi = bsi_start (bb); !bsi_end_p (bsi); bsi_next (&bsi))
-	{
-	  tree stmt = bsi_stmt (bsi);
-	  if (tree_can_make_abnormal_goto (stmt))
-	    {
-	      if (stmt == bsi_stmt (bsi_last (bb)))
-		{
-		  if (!has_abnormal_outgoing_edge_p (bb))
-		    make_abnormal_goto_edges (bb, true);
-		}
-	      else
-		{
-		  edge e = split_block (bb, stmt);
-		  bb = e->src;
-		  make_abnormal_goto_edges (bb, true);
-		}
-	      break;
-	    }
-
-	  /* Update PHIs on nonlocal goto receivers we (possibly)
-	     just created new edges into.  */
-	  if (TREE_CODE (stmt) == LABEL_EXPR
-	      && gimple_in_ssa_p (cfun))
-	    {
-	      tree target = LABEL_EXPR_LABEL (stmt);
-	      if (DECL_NONLOCAL (target))
-		{
-		  tree phi;
-
-		  for (phi = phi_nodes (bb); phi; phi = PHI_CHAIN (phi))
-		    {
-		      gcc_assert (SSA_NAME_OCCURS_IN_ABNORMAL_PHI
-				  (PHI_RESULT (phi)));
-		      mark_sym_for_renaming
-			(SSA_NAME_VAR (PHI_RESULT (phi)));
-		    }
-		}
-	    }
-	}
-    }
-}
-
 /* Expand calls to inline functions in the body of FN.  */
 
 unsigned int
@@ -2935,8 +2914,6 @@ optimize_inline_calls (tree fn)
   cgraph_node_remove_callees (id.dst_node);
 
   fold_cond_expr_cond ();
-  if (current_function_has_nonlocal_label)
-    make_nonlocal_label_edges ();
   /* It would be nice to check SSA/CFG/statement consistency here, but it is
      not possible yet - the IPA passes might make various functions to not
      throw and they don't care to proactively update local EH info.  This is
Index: gcc/tree-cfg.c
===================================================================
--- gcc/tree-cfg.c.orig	2007-09-28 00:03:12.000000000 -0300
+++ gcc/tree-cfg.c	2007-09-28 00:03:16.000000000 -0300
@@ -4258,11 +4258,18 @@ verify_stmts (void)
 	      tree t = PHI_ARG_DEF (phi, i);
 	      tree addr;
 
+	      if (!t)
+		{
+		  error ("missing PHI def");
+		  debug_generic_stmt (phi);
+		  err |= true;
+		  continue;
+		}
 	      /* Addressable variables do have SSA_NAMEs but they
 		 are not considered gimple values.  */
-	      if (TREE_CODE (t) != SSA_NAME
-		  && TREE_CODE (t) != FUNCTION_DECL
-		  && !is_gimple_val (t))
+	      else if (TREE_CODE (t) != SSA_NAME
+		       && TREE_CODE (t) != FUNCTION_DECL
+		       && !is_gimple_val (t))
 		{
 		  error ("PHI def is not a GIMPLE value");
 		  debug_generic_stmt (phi);

[-- Attachment #3: Type: text/plain, Size: 249 bytes --]


-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: [vtab (and trunk?)] fix fallout from SSA-coalescing user variables

[Andrew Pinski]

On 10/1/07, Alexandre Oliva <aoliva@redhat.com> wrote:
> This patch arranges for us to note the correct variable version to use
> at the new edges for such gimple registers, instead of incorrectly
> relying on renaming to do that.

I think this is also PR 33572.

Thanks,
Andrew Pinski

Re: [vtab (and trunk?)] fix fallout from SSA-coalescing user variables

[Eric Botcazou]

> As it turns out, the compiler was not ready for such coalescing of
> gimple registers.  It created situations in which, because of the
> abnormal setjmp/longjmp edges used for EH in the (stage1?) Ada
> compiler, we'd get variables that were gimple registers and that had
> multiple live versions simultaneously renamed at the time of
> inlining.

The EH scheme doesn't depend on the stage, it's __builtin_setjmp/longjmp.

-- 
Eric Botcazou

Re: [vtab (and trunk?)] fix fallout from SSA-coalescing user variables

[Richard Guenther]

On 10/1/07, Alexandre Oliva <aoliva@redhat.com> wrote:
> After installing the patch that enables SSA coalescing of all
> variables, I've experienced some compilation errors in Ada, having to
> do with EH tables.
>
> As it turns out, the compiler was not ready for such coalescing of
> gimple registers.  It created situations in which, because of the
> abnormal setjmp/longjmp edges used for EH in the (stage1?) Ada
> compiler, we'd get variables that were gimple registers and that had
> multiple live versions simultaneously renamed at the time of
> inlining.  This should never happen.
>
> This patch arranges for us to note the correct variable version to use
> at the new edges for such gimple registers, instead of incorrectly
> relying on renaming to do that.

I think you need to update the comment before update_ssa_across_abnormal_edges.
Can you take care of bootstrapping/testing this, check if it fixes
PR33572 and add
the testcase?

Thanks,
Richard.

Re: [vtab (and trunk?)] fix fallout from SSA-coalescing user variables

[Alexandre Oliva]

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

On Oct  2, 2007, "Richard Guenther" <richard.guenther@gmail.com> wrote:

> I think you need to update the comment before
> update_ssa_across_abnormal_edges.  Can you take care of
> bootstrapping/testing this, check if it fixes PR33572 and add the
> testcase?

Done, here's the patch for mainline.  Bootstrapped and regtested on
x86_64-linux-gnu, no regressions.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-debug-coalesce-inline-fallout.patch --]
[-- Type: text/x-patch, Size: 8938 bytes --]

for gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR tree-optimization/33572
	* tree-cfg.c (verify_stmts): Check for missing PHI defs.
	* tree-inline.c (update_ssa_across_eh_edges): Renamed to...
	(update_ssa_across_abnormal_edges): ... this.  Set slots in the
	return PHI node.
	(copy_edges_for_bb): Handle nonlocal label edges.
	(make_nonlocal_label_edges): Deleted.
	(optimize_inline_calls): Don't call it.

for gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR tree-optimization/33572
	* g++.dg/tree-ssa/pr33572.C: New.

Index: gcc/tree-inline.c
===================================================================
--- gcc/tree-inline.c.orig	2007-10-02 22:00:02.000000000 -0300
+++ gcc/tree-inline.c	2007-10-02 22:22:35.000000000 -0300
@@ -1009,16 +1009,19 @@ copy_bb (copy_body_data *id, basic_block
    across EH edges from basic block within inlined functions destinating
    to landing pads in function we inline into.
 
-   The function mark PHI_RESULT of such PHI nodes for renaming; it is
-   safe the EH edges are abnormal and SSA_NAME_OCCURS_IN_ABNORMAL_PHI
-   must be set.  This means, that there will be no overlapping live ranges
+   The function fills in PHI_RESULTs of such PHI nodes if they refer
+   to gimple regs.  Otherwise, the function mark PHI_RESULT of such
+   PHI nodes for renaming.  For non-gimple regs, renaming is safe: the
+   EH edges are abnormal and SSA_NAME_OCCURS_IN_ABNORMAL_PHI must be
+   set, and this means that there will be no overlapping live ranges
    for the underlying symbol.
 
    This might change in future if we allow redirecting of EH edges and
    we might want to change way build CFG pre-inlining to include
    all the possible edges then.  */
 static void
-update_ssa_across_eh_edges (basic_block bb)
+update_ssa_across_abnormal_edges (basic_block bb, basic_block ret_bb,
+				  bool can_throw, bool nonlocal_goto)
 {
   edge e;
   edge_iterator ei;
@@ -1029,13 +1032,35 @@ update_ssa_across_eh_edges (basic_block 
       {
 	tree phi;
 
-	gcc_assert (e->flags & EDGE_EH);
+	gcc_assert (e->flags & EDGE_ABNORMAL);
+	if (!nonlocal_goto)
+	  gcc_assert (e->flags & EDGE_EH);
+	if (!can_throw)
+	  gcc_assert (!(e->flags & EDGE_EH));
 	for (phi = phi_nodes (e->dest); phi; phi = PHI_CHAIN (phi))
 	  {
+	    edge re;
+
+	    /* There shouldn't be any PHI nodes in the ENTRY_BLOCK.  */
+	    gcc_assert (!e->dest->aux);
+
 	    gcc_assert (SSA_NAME_OCCURS_IN_ABNORMAL_PHI
 			(PHI_RESULT (phi)));
-	    mark_sym_for_renaming
-	      (SSA_NAME_VAR (PHI_RESULT (phi)));
+
+	    if (!is_gimple_reg (PHI_RESULT (phi)))
+	      {
+		mark_sym_for_renaming
+		  (SSA_NAME_VAR (PHI_RESULT (phi)));
+		continue;
+	      }
+
+	    re = find_edge (ret_bb, e->dest);
+	    gcc_assert (re);
+	    gcc_assert ((re->flags & (EDGE_EH | EDGE_ABNORMAL))
+			== (e->flags & (EDGE_EH | EDGE_ABNORMAL)));
+
+	    SET_USE (PHI_ARG_DEF_PTR_FROM_EDGE (phi, e),
+		     USE_FROM_PTR (PHI_ARG_DEF_PTR_FROM_EDGE (phi, re)));
 	  }
       }
 }
@@ -1044,7 +1069,7 @@ update_ssa_across_eh_edges (basic_block 
    accordingly.  Edges will be taken care of later.  Assume aux
    pointers to point to the copies of each BB.  */
 static void
-copy_edges_for_bb (basic_block bb, int count_scale)
+copy_edges_for_bb (basic_block bb, int count_scale, basic_block ret_bb)
 {
   basic_block new_bb = (basic_block) bb->aux;
   edge_iterator ei;
@@ -1076,6 +1101,7 @@ copy_edges_for_bb (basic_block bb, int c
   for (bsi = bsi_start (new_bb); !bsi_end_p (bsi);)
     {
       tree copy_stmt;
+      bool can_throw, nonlocal_goto;
 
       copy_stmt = bsi_stmt (bsi);
       update_stmt (copy_stmt);
@@ -1096,7 +1122,10 @@ copy_edges_for_bb (basic_block bb, int c
          into a COMPONENT_REF which doesn't.  If the copy
          can throw, the original could also throw.  */
 
-      if (tree_can_throw_internal (copy_stmt))
+      can_throw = tree_can_throw_internal (copy_stmt);
+      nonlocal_goto = tree_can_make_abnormal_goto (copy_stmt);
+
+      if (can_throw || nonlocal_goto)
 	{
 	  if (!bsi_end_p (bsi))
 	    /* Note that bb's predecessor edges aren't necessarily
@@ -1108,12 +1137,18 @@ copy_edges_for_bb (basic_block bb, int c
 	      new_bb->aux = e->src->aux;
 	      bsi = bsi_start (new_bb);
 	    }
+	}
 
-           make_eh_edges (copy_stmt);
+      if (can_throw)
+	make_eh_edges (copy_stmt);
 
-	   if (gimple_in_ssa_p (cfun))
-	     update_ssa_across_eh_edges (bb_for_stmt (copy_stmt));
-	}
+      if (nonlocal_goto)
+	make_abnormal_goto_edges (bb_for_stmt (copy_stmt), true);
+
+      if ((can_throw || nonlocal_goto)
+	  && gimple_in_ssa_p (cfun))
+	update_ssa_across_abnormal_edges (bb_for_stmt (copy_stmt), ret_bb,
+					  can_throw, nonlocal_goto);
     }
 }
 
@@ -1285,7 +1320,7 @@ copy_cfg_body (copy_body_data * id, gcov
   last = last_basic_block;
   /* Now that we've duplicated the blocks, duplicate their edges.  */
   FOR_ALL_BB_FN (bb, cfun_to_copy)
-    copy_edges_for_bb (bb, count_scale);
+    copy_edges_for_bb (bb, count_scale, exit_block_map);
   if (gimple_in_ssa_p (cfun))
     FOR_ALL_BB_FN (bb, cfun_to_copy)
       copy_phis_for_bb (bb, id);
@@ -2803,60 +2838,6 @@ has_abnormal_outgoing_edge_p (basic_bloc
   return false;
 }
 
-/* When a block from the inlined function contains a call with side-effects
-   in the middle gets inlined in a function with non-locals labels, the call
-   becomes a potential non-local goto so we need to add appropriate edge.  */
-
-static void
-make_nonlocal_label_edges (void)
-{
-  block_stmt_iterator bsi;
-  basic_block bb;
-
-  FOR_EACH_BB (bb)
-    {
-      for (bsi = bsi_start (bb); !bsi_end_p (bsi); bsi_next (&bsi))
-	{
-	  tree stmt = bsi_stmt (bsi);
-	  if (tree_can_make_abnormal_goto (stmt))
-	    {
-	      if (stmt == bsi_stmt (bsi_last (bb)))
-		{
-		  if (!has_abnormal_outgoing_edge_p (bb))
-		    make_abnormal_goto_edges (bb, true);
-		}
-	      else
-		{
-		  edge e = split_block (bb, stmt);
-		  bb = e->src;
-		  make_abnormal_goto_edges (bb, true);
-		}
-	      break;
-	    }
-
-	  /* Update PHIs on nonlocal goto receivers we (possibly)
-	     just created new edges into.  */
-	  if (TREE_CODE (stmt) == LABEL_EXPR
-	      && gimple_in_ssa_p (cfun))
-	    {
-	      tree target = LABEL_EXPR_LABEL (stmt);
-	      if (DECL_NONLOCAL (target))
-		{
-		  tree phi;
-
-		  for (phi = phi_nodes (bb); phi; phi = PHI_CHAIN (phi))
-		    {
-		      gcc_assert (SSA_NAME_OCCURS_IN_ABNORMAL_PHI
-				  (PHI_RESULT (phi)));
-		      mark_sym_for_renaming
-			(SSA_NAME_VAR (PHI_RESULT (phi)));
-		    }
-		}
-	    }
-	}
-    }
-}
-
 /* Expand calls to inline functions in the body of FN.  */
 
 unsigned int
@@ -2935,8 +2916,6 @@ optimize_inline_calls (tree fn)
   cgraph_node_remove_callees (id.dst_node);
 
   fold_cond_expr_cond ();
-  if (current_function_has_nonlocal_label)
-    make_nonlocal_label_edges ();
   /* It would be nice to check SSA/CFG/statement consistency here, but it is
      not possible yet - the IPA passes might make various functions to not
      throw and they don't care to proactively update local EH info.  This is
Index: gcc/tree-cfg.c
===================================================================
--- gcc/tree-cfg.c.orig	2007-10-02 22:00:02.000000000 -0300
+++ gcc/tree-cfg.c	2007-10-02 22:16:45.000000000 -0300
@@ -4258,11 +4258,18 @@ verify_stmts (void)
 	      tree t = PHI_ARG_DEF (phi, i);
 	      tree addr;
 
+	      if (!t)
+		{
+		  error ("missing PHI def");
+		  debug_generic_stmt (phi);
+		  err |= true;
+		  continue;
+		}
 	      /* Addressable variables do have SSA_NAMEs but they
 		 are not considered gimple values.  */
-	      if (TREE_CODE (t) != SSA_NAME
-		  && TREE_CODE (t) != FUNCTION_DECL
-		  && !is_gimple_val (t))
+	      else if (TREE_CODE (t) != SSA_NAME
+		       && TREE_CODE (t) != FUNCTION_DECL
+		       && !is_gimple_val (t))
 		{
 		  error ("PHI def is not a GIMPLE value");
 		  debug_generic_stmt (phi);
Index: gcc/testsuite/g++.dg/tree-ssa/pr33572.C
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/g++.dg/tree-ssa/pr33572.C	2007-10-02 22:16:45.000000000 -0300
@@ -0,0 +1,39 @@
+/* { dg-do run } */
+/* { dg-options "-O" } */
+
+namespace __gnu_cxx {
+	template<bool> struct __pool {
+		void _M_reclaim_block(char* p, unsigned long bytes);
+	};
+}
+
+struct vector {
+	~vector() { deallocate(0); }
+	void deallocate(int* p) {
+		if (p) {
+			static __gnu_cxx::__pool<true> pool;
+			pool._M_reclaim_block((char*)0, 0);
+		}
+	}
+};
+
+struct Foo { virtual void f() { } };
+
+struct auto_ptr {
+	Foo* ptr;
+	auto_ptr() : ptr(0) { }
+	~auto_ptr() { delete ptr; }
+	Foo* release() { Foo* tmp = ptr; ptr = 0; return tmp; }
+	void reset(Foo* p) { ptr = p; }
+};
+
+int main(int argc, char**) {
+	auto_ptr foo;
+	if (argc) {
+		foo.reset(new Foo());
+	} else {
+		vector v;
+	}
+	Foo* p = foo.release();
+	p->f();
+}

[-- Attachment #3: Type: text/plain, Size: 249 bytes --]


-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: [vtab (and trunk?)] fix fallout from SSA-coalescing user variables

[Alexandre Oliva]

On Oct  1, 2007, Eric Botcazou <ebotcazou@adacore.com> wrote:

> The EH scheme doesn't depend on the stage, it's __builtin_setjmp/longjmp.

I distinctly remember having observed differences in the EH scheme
depending on how far into the build I was.  E.g., after building the
Ada libraries, it would use a different EH scheme, so the very same
testcase with the same compilation options would not trigger the bug.
So I wiped out the tree, started another build after fixing some other
problems, there was the bug again, with the stage1 compiler.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: [vtab (and trunk?)] fix fallout from SSA-coalescing user variables

[Eric Botcazou]

> I distinctly remember having observed differences in the EH scheme
> depending on how far into the build I was.  E.g., after building the
> Ada libraries, it would use a different EH scheme, so the very same
> testcase with the same compilation options would not trigger the bug.

The 3 compilers behave the same, all other things being equal.  They use 
__builtin_setjmp/longjmp by default (in particular when self-compiling, the 
Ada compiler uses EH internally) unless they have access to a system.ads file 
that overrides this default.  So, yes, once the library is built, the default 
is overridden, but you can go back to this default by replacing system.ads 
with the one from the source tree (e.g. -I- -I/home/eric/svn/gcc/gcc/ada).

-- 
Eric Botcazou

Re: [vtab (and trunk?)] fix fallout from SSA-coalescing user variables

[Richard Guenther]

On 10/3/07, Alexandre Oliva <aoliva@redhat.com> wrote:
> > I think you need to update the comment before
> > update_ssa_across_abnormal_edges.  Can you take care of
> > bootstrapping/testing this, check if it fixes PR33572 and add the
> > testcase?
>
> Done, here's the patch for mainline.  Bootstrapped and regtested on
> x86_64-linux-gnu, no regressions.

This is ok if you move the testcase from g++.dg/tree-ssa to g++.dg/torture
(and remove the dg-options then).

Thanks,
Richard.

Re: [vtab (and trunk?)] fix fallout from SSA-coalescing user variables

[Alexandre Oliva]

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

On Oct  3, 2007, "Richard Guenther" <richard.guenther@gmail.com> wrote:

> On 10/3/07, Alexandre Oliva <aoliva@redhat.com> wrote:
>> > I think you need to update the comment before
>> > update_ssa_across_abnormal_edges.  Can you take care of
>> > bootstrapping/testing this, check if it fixes PR33572 and add the
>> > testcase?
>> 
>> Done, here's the patch for mainline.  Bootstrapped and regtested on
>> x86_64-linux-gnu, no regressions.

> This is ok if you move the testcase from g++.dg/tree-ssa to g++.dg/torture
> (and remove the dg-options then).

Thanks, here's what I've just installed in the trunk.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-debug-coalesce-inline-fallout.patch --]
[-- Type: text/x-patch, Size: 8903 bytes --]

for gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR tree-optimization/33572
	* tree-cfg.c (verify_stmts): Check for missing PHI defs.
	* tree-inline.c (update_ssa_across_eh_edges): Renamed to...
	(update_ssa_across_abnormal_edges): ... this.  Set slots in the
	return PHI node.
	(copy_edges_for_bb): Handle nonlocal label edges.
	(make_nonlocal_label_edges): Deleted.
	(optimize_inline_calls): Don't call it.

for gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR tree-optimization/33572
	* g++.dg/torture/pr33572.C: New.

Index: gcc/tree-inline.c
===================================================================
--- gcc/tree-inline.c.orig	2007-10-02 22:00:02.000000000 -0300
+++ gcc/tree-inline.c	2007-10-02 22:22:35.000000000 -0300
@@ -1009,16 +1009,19 @@ copy_bb (copy_body_data *id, basic_block
    across EH edges from basic block within inlined functions destinating
    to landing pads in function we inline into.
 
-   The function mark PHI_RESULT of such PHI nodes for renaming; it is
-   safe the EH edges are abnormal and SSA_NAME_OCCURS_IN_ABNORMAL_PHI
-   must be set.  This means, that there will be no overlapping live ranges
+   The function fills in PHI_RESULTs of such PHI nodes if they refer
+   to gimple regs.  Otherwise, the function mark PHI_RESULT of such
+   PHI nodes for renaming.  For non-gimple regs, renaming is safe: the
+   EH edges are abnormal and SSA_NAME_OCCURS_IN_ABNORMAL_PHI must be
+   set, and this means that there will be no overlapping live ranges
    for the underlying symbol.
 
    This might change in future if we allow redirecting of EH edges and
    we might want to change way build CFG pre-inlining to include
    all the possible edges then.  */
 static void
-update_ssa_across_eh_edges (basic_block bb)
+update_ssa_across_abnormal_edges (basic_block bb, basic_block ret_bb,
+				  bool can_throw, bool nonlocal_goto)
 {
   edge e;
   edge_iterator ei;
@@ -1029,13 +1032,35 @@ update_ssa_across_eh_edges (basic_block 
       {
 	tree phi;
 
-	gcc_assert (e->flags & EDGE_EH);
+	gcc_assert (e->flags & EDGE_ABNORMAL);
+	if (!nonlocal_goto)
+	  gcc_assert (e->flags & EDGE_EH);
+	if (!can_throw)
+	  gcc_assert (!(e->flags & EDGE_EH));
 	for (phi = phi_nodes (e->dest); phi; phi = PHI_CHAIN (phi))
 	  {
+	    edge re;
+
+	    /* There shouldn't be any PHI nodes in the ENTRY_BLOCK.  */
+	    gcc_assert (!e->dest->aux);
+
 	    gcc_assert (SSA_NAME_OCCURS_IN_ABNORMAL_PHI
 			(PHI_RESULT (phi)));
-	    mark_sym_for_renaming
-	      (SSA_NAME_VAR (PHI_RESULT (phi)));
+
+	    if (!is_gimple_reg (PHI_RESULT (phi)))
+	      {
+		mark_sym_for_renaming
+		  (SSA_NAME_VAR (PHI_RESULT (phi)));
+		continue;
+	      }
+
+	    re = find_edge (ret_bb, e->dest);
+	    gcc_assert (re);
+	    gcc_assert ((re->flags & (EDGE_EH | EDGE_ABNORMAL))
+			== (e->flags & (EDGE_EH | EDGE_ABNORMAL)));
+
+	    SET_USE (PHI_ARG_DEF_PTR_FROM_EDGE (phi, e),
+		     USE_FROM_PTR (PHI_ARG_DEF_PTR_FROM_EDGE (phi, re)));
 	  }
       }
 }
@@ -1044,7 +1069,7 @@ update_ssa_across_eh_edges (basic_block 
    accordingly.  Edges will be taken care of later.  Assume aux
    pointers to point to the copies of each BB.  */
 static void
-copy_edges_for_bb (basic_block bb, int count_scale)
+copy_edges_for_bb (basic_block bb, int count_scale, basic_block ret_bb)
 {
   basic_block new_bb = (basic_block) bb->aux;
   edge_iterator ei;
@@ -1076,6 +1101,7 @@ copy_edges_for_bb (basic_block bb, int c
   for (bsi = bsi_start (new_bb); !bsi_end_p (bsi);)
     {
       tree copy_stmt;
+      bool can_throw, nonlocal_goto;
 
       copy_stmt = bsi_stmt (bsi);
       update_stmt (copy_stmt);
@@ -1096,7 +1122,10 @@ copy_edges_for_bb (basic_block bb, int c
          into a COMPONENT_REF which doesn't.  If the copy
          can throw, the original could also throw.  */
 
-      if (tree_can_throw_internal (copy_stmt))
+      can_throw = tree_can_throw_internal (copy_stmt);
+      nonlocal_goto = tree_can_make_abnormal_goto (copy_stmt);
+
+      if (can_throw || nonlocal_goto)
 	{
 	  if (!bsi_end_p (bsi))
 	    /* Note that bb's predecessor edges aren't necessarily
@@ -1108,12 +1137,18 @@ copy_edges_for_bb (basic_block bb, int c
 	      new_bb->aux = e->src->aux;
 	      bsi = bsi_start (new_bb);
 	    }
+	}
 
-           make_eh_edges (copy_stmt);
+      if (can_throw)
+	make_eh_edges (copy_stmt);
 
-	   if (gimple_in_ssa_p (cfun))
-	     update_ssa_across_eh_edges (bb_for_stmt (copy_stmt));
-	}
+      if (nonlocal_goto)
+	make_abnormal_goto_edges (bb_for_stmt (copy_stmt), true);
+
+      if ((can_throw || nonlocal_goto)
+	  && gimple_in_ssa_p (cfun))
+	update_ssa_across_abnormal_edges (bb_for_stmt (copy_stmt), ret_bb,
+					  can_throw, nonlocal_goto);
     }
 }
 
@@ -1285,7 +1320,7 @@ copy_cfg_body (copy_body_data * id, gcov
   last = last_basic_block;
   /* Now that we've duplicated the blocks, duplicate their edges.  */
   FOR_ALL_BB_FN (bb, cfun_to_copy)
-    copy_edges_for_bb (bb, count_scale);
+    copy_edges_for_bb (bb, count_scale, exit_block_map);
   if (gimple_in_ssa_p (cfun))
     FOR_ALL_BB_FN (bb, cfun_to_copy)
       copy_phis_for_bb (bb, id);
@@ -2803,60 +2838,6 @@ has_abnormal_outgoing_edge_p (basic_bloc
   return false;
 }
 
-/* When a block from the inlined function contains a call with side-effects
-   in the middle gets inlined in a function with non-locals labels, the call
-   becomes a potential non-local goto so we need to add appropriate edge.  */
-
-static void
-make_nonlocal_label_edges (void)
-{
-  block_stmt_iterator bsi;
-  basic_block bb;
-
-  FOR_EACH_BB (bb)
-    {
-      for (bsi = bsi_start (bb); !bsi_end_p (bsi); bsi_next (&bsi))
-	{
-	  tree stmt = bsi_stmt (bsi);
-	  if (tree_can_make_abnormal_goto (stmt))
-	    {
-	      if (stmt == bsi_stmt (bsi_last (bb)))
-		{
-		  if (!has_abnormal_outgoing_edge_p (bb))
-		    make_abnormal_goto_edges (bb, true);
-		}
-	      else
-		{
-		  edge e = split_block (bb, stmt);
-		  bb = e->src;
-		  make_abnormal_goto_edges (bb, true);
-		}
-	      break;
-	    }
-
-	  /* Update PHIs on nonlocal goto receivers we (possibly)
-	     just created new edges into.  */
-	  if (TREE_CODE (stmt) == LABEL_EXPR
-	      && gimple_in_ssa_p (cfun))
-	    {
-	      tree target = LABEL_EXPR_LABEL (stmt);
-	      if (DECL_NONLOCAL (target))
-		{
-		  tree phi;
-
-		  for (phi = phi_nodes (bb); phi; phi = PHI_CHAIN (phi))
-		    {
-		      gcc_assert (SSA_NAME_OCCURS_IN_ABNORMAL_PHI
-				  (PHI_RESULT (phi)));
-		      mark_sym_for_renaming
-			(SSA_NAME_VAR (PHI_RESULT (phi)));
-		    }
-		}
-	    }
-	}
-    }
-}
-
 /* Expand calls to inline functions in the body of FN.  */
 
 unsigned int
@@ -2935,8 +2916,6 @@ optimize_inline_calls (tree fn)
   cgraph_node_remove_callees (id.dst_node);
 
   fold_cond_expr_cond ();
-  if (current_function_has_nonlocal_label)
-    make_nonlocal_label_edges ();
   /* It would be nice to check SSA/CFG/statement consistency here, but it is
      not possible yet - the IPA passes might make various functions to not
      throw and they don't care to proactively update local EH info.  This is
Index: gcc/tree-cfg.c
===================================================================
--- gcc/tree-cfg.c.orig	2007-10-02 22:00:02.000000000 -0300
+++ gcc/tree-cfg.c	2007-10-02 22:16:45.000000000 -0300
@@ -4258,11 +4258,18 @@ verify_stmts (void)
 	      tree t = PHI_ARG_DEF (phi, i);
 	      tree addr;
 
+	      if (!t)
+		{
+		  error ("missing PHI def");
+		  debug_generic_stmt (phi);
+		  err |= true;
+		  continue;
+		}
 	      /* Addressable variables do have SSA_NAMEs but they
 		 are not considered gimple values.  */
-	      if (TREE_CODE (t) != SSA_NAME
-		  && TREE_CODE (t) != FUNCTION_DECL
-		  && !is_gimple_val (t))
+	      else if (TREE_CODE (t) != SSA_NAME
+		       && TREE_CODE (t) != FUNCTION_DECL
+		       && !is_gimple_val (t))
 		{
 		  error ("PHI def is not a GIMPLE value");
 		  debug_generic_stmt (phi);
Index: gcc/testsuite/g++.dg/torture/pr33572.C
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/g++.dg/torture/pr33572.C	2007-10-02 22:16:45.000000000 -0300
@@ -0,0 +1,37 @@
+// { dg-do run }
+namespace __gnu_cxx {
+	template<bool> struct __pool {
+		void _M_reclaim_block(char* p, unsigned long bytes);
+	};
+}
+
+struct vector {
+	~vector() { deallocate(0); }
+	void deallocate(int* p) {
+		if (p) {
+			static __gnu_cxx::__pool<true> pool;
+			pool._M_reclaim_block((char*)0, 0);
+		}
+	}
+};
+
+struct Foo { virtual void f() { } };
+
+struct auto_ptr {
+	Foo* ptr;
+	auto_ptr() : ptr(0) { }
+	~auto_ptr() { delete ptr; }
+	Foo* release() { Foo* tmp = ptr; ptr = 0; return tmp; }
+	void reset(Foo* p) { ptr = p; }
+};
+
+int main(int argc, char**) {
+	auto_ptr foo;
+	if (argc) {
+		foo.reset(new Foo());
+	} else {
+		vector v;
+	}
+	Foo* p = foo.release();
+	p->f();
+}

[-- Attachment #3: Type: text/plain, Size: 249 bytes --]


-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Regression with "Re: [vtab (and trunk?)] fix fallout from SSA-coalescing user variables"

[Hans-Peter Nilsson]

> From: Alexandre Oliva <aoliva@redhat.com>
> Date: Sat, 06 Oct 2007 08:45:00 -0300

> from  Alexandre Oliva  <aoliva@redhat.com>
> 
> 	PR tree-optimization/33572
> 	* tree-cfg.c (verify_stmts): Check for missing PHI defs.
> 	* tree-inline.c (update_ssa_across_eh_edges): Renamed to...
> 	(update_ssa_across_abnormal_edges): ... this.  Set slots in the
> 	return PHI node.
> 	(copy_edges_for_bb): Handle nonlocal label edges.
> 	(make_nonlocal_label_edges): Deleted.
> 	(optimize_inline_calls): Don't call it.

A change in the range 129038:129054, where the only suspects are
the change above and theoretically your subsequent:

       PR tree-optimization/33655
       * tree-sra.c (bitfield_overlaps_p): Handle array and complex
       elements.

seems to have caused this regression for cris-elf:

Running /tmp/hpautotest-gcc1/gcc/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp ...
...
FAIL: ext/pb_ds/regression/hash_no_data_map_rand.cc (test for excess errors)

with the log saying:

/tmp/hpautotest-gcc1/gcc/libstdc++-v3/testsuite/util/regression/rand/assoc/detail/constructor_destructor_fn_imps.hpp: In member function 'bool __gnu_pbds::test::detail::container_rand_regression_test<Cntnr>::it_constructor_imp(__gnu_pbds::cc_hash_tag) [with Cntnr = __gnu_pbds::cc_hash_table<__gnu_pbds::test::basic_type, __gnu_pbds::null_mapped_type, __gnu_pbds::test::hash, std::equal_to<__gnu_pbds::test::basic_type>, __gnu_pbds::test::direct_mask_range_hashing_t_<__gnu_cxx::throw_allocator<__gnu_pbds::test::basic_type> >, __gnu_pbds::hash_standard_resize_policy<__gnu_pbds::test::hash_exponential_size_policy_t_<__gnu_cxx::throw_allocator<__gnu_pbds::test::basic_type> >, __gnu_pbds::test::hash_load_check_resize_trigger_t_<__gnu_cxx::throw_allocator<__gnu_pbds::test::basic_type>, 1u, 8u, 1u, 2u, true>, true, long unsigned int>, false, __gnu_cxx::throw_allocator<__gnu_pbds::test::basic_type> >]':
/tmp/hpautotest-gcc1/gcc/libstdc++-v3/testsuite/util/regression/rand/assoc/detail/constructor_destructor_fn_imps.hpp:215: internal compiler error: in update_ssa_across_abnormal_edges, at tree-inline.c:1058

brgds, H-P

Re: Regression with "Re: [vtab (and trunk?)] fix fallout from SSA-coalescing user variables"

[Alexandre Oliva]

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

On Oct  6, 2007, Hans-Peter Nilsson <hans-peter.nilsson@axis.com> wrote:

> Running /tmp/hpautotest-gcc1/gcc/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp ...
> ...
> FAIL: ext/pb_ds/regression/hash_no_data_map_rand.cc (test for excess errors)

> /tmp/hpautotest-gcc1/gcc/libstdc++-v3/testsuite/util/regression/rand/assoc/detail/constructor_destructor_fn_imps.hpp:215: internal compiler error: in update_ssa_across_abnormal_edges, at tree-inline.c:1058

AFAICT, this error is a result of unnecessary abnormal edges being
conservatively inserted and then removed before the end of the pass.
This patch fixes the too-conservative assertion I'd added.  If we ever
run into a case in which the edge is not removed before the end of the
pass, the verification code I've added in the previous patch will
catch it.

Ok to install?  Build-tested on x86_64-linux-gnu, no regressions are
possible since all it does is to relax an assertion that had never hit
before and that would have caused an instant ICE.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-pr33572-fallout-cris.patch --]
[-- Type: text/x-patch, Size: 705 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR tree-optimization/33572
	* tree-inline.c (update_ssa_across_abnormal_edges): Tolerate
	the absence of a corresponding edge from the exit block.

Index: gcc/tree-inline.c
===================================================================
--- gcc/tree-inline.c.orig	2007-10-08 15:41:47.000000000 -0300
+++ gcc/tree-inline.c	2007-10-08 15:42:42.000000000 -0300
@@ -1055,7 +1055,8 @@ update_ssa_across_abnormal_edges (basic_
 	      }
 
 	    re = find_edge (ret_bb, e->dest);
-	    gcc_assert (re);
+	    if (!re)
+	      continue;
 	    gcc_assert ((re->flags & (EDGE_EH | EDGE_ABNORMAL))
 			== (e->flags & (EDGE_EH | EDGE_ABNORMAL)));
 

[-- Attachment #3: Type: text/plain, Size: 250 bytes --]



-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: [vtab (and trunk?)] fix fallout from SSA-coalescing user variables

[Alexandre Oliva]

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

On Oct  6, 2007, Alexandre Oliva <aoliva@redhat.com> wrote:

> On Oct  3, 2007, "Richard Guenther" <richard.guenther@gmail.com> wrote:

>> This is ok if you move the testcase from g++.dg/tree-ssa to g++.dg/torture
>> (and remove the dg-options then).

> Thanks, here's what I've just installed in the trunk.

Somehow the reduced testcase failed because of the undefined function
on some platforms, even though it didn't fail on x86_64-linux-gnu.

Here's a patch that replaces it with the original testcase in the bug
report.

Ok to install?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-pr33572-fallout-test.patch --]
[-- Type: text/x-patch, Size: 1306 bytes --]

for gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR tree-optimization/33572
	* g++.dg/torture/pr33572.C: Replace with complete test.

Index: gcc/testsuite/g++.dg/torture/pr33572.C
===================================================================
--- gcc/testsuite/g++.dg/torture/pr33572.C.orig	2007-10-08 17:33:33.000000000 -0300
+++ gcc/testsuite/g++.dg/torture/pr33572.C	2007-10-08 17:34:17.000000000 -0300
@@ -1,36 +1,16 @@
 // { dg-do run }
-namespace __gnu_cxx {
-	template<bool> struct __pool {
-		void _M_reclaim_block(char* p, unsigned long bytes);
-	};
-}
-
-struct vector {
-	~vector() { deallocate(0); }
-	void deallocate(int* p) {
-		if (p) {
-			static __gnu_cxx::__pool<true> pool;
-			pool._M_reclaim_block((char*)0, 0);
-		}
-	}
-};
-
-struct Foo { virtual void f() { } };
+#include <vector>
+#include <memory>
 
-struct auto_ptr {
-	Foo* ptr;
-	auto_ptr() : ptr(0) { }
-	~auto_ptr() { delete ptr; }
-	Foo* release() { Foo* tmp = ptr; ptr = 0; return tmp; }
-	void reset(Foo* p) { ptr = p; }
-};
+struct Foo { virtual void f() {} };
 
-int main(int argc, char**) {
-	auto_ptr foo;
+int main(int argc, char**)
+{
+	std::auto_ptr<Foo> foo;
 	if (argc) {
 		foo.reset(new Foo());
 	} else {
-		vector v;
+		std::vector<int> v;
 	}
 	Foo* p = foo.release();
 	p->f();

[-- Attachment #3: Type: text/plain, Size: 249 bytes --]


-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: [vtab (and trunk?)] fix fallout from SSA-coalescing user variables

[Alexandre Oliva]

On Oct  8, 2007, Alexandre Oliva <aoliva@redhat.com> wrote:

> for gcc/testsuite/ChangeLog
> from  Alexandre Oliva  <aoliva@redhat.com>

> 	PR tree-optimization/33572
> 	* g++.dg/torture/pr33572.C: Replace with complete test.

I'm going ahead and checking this in as obviously correct.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Regression with "Re: [vtab (and trunk?)] fix fallout from SSA-coalescing user variables"

[Alexandre Oliva]

On Oct  8, 2007, Alexandre Oliva <aoliva@redhat.com> wrote:

> Ok to install?  Build-tested on x86_64-linux-gnu, no regressions are
> possible since all it does is to relax an assertion that had never hit
> before and that would have caused an instant ICE.


> for  gcc/ChangeLog
> from  Alexandre Oliva  <aoliva@redhat.com>

> 	PR tree-optimization/33572
> 	* tree-inline.c (update_ssa_across_abnormal_edges): Tolerate
> 	the absence of a corresponding edge from the exit block.

I'm going ahead and checking this in for now.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: [vtab (and trunk?)] fix fallout from SSA-coalescing user variables

[Richard Guenther]

On 10/6/07, Alexandre Oliva <aoliva@redhat.com> wrote
> > On 10/3/07, Alexandre Oliva <aoliva@redhat.com> wrote:
> >> > I think you need to update the comment before
> >> > update_ssa_across_abnormal_edges.  Can you take care of
> >> > bootstrapping/testing this, check if it fixes PR33572 and add the
> >> > testcase?
> >>
> >> Done, here's the patch for mainline.  Bootstrapped and regtested on
> >> x86_64-linux-gnu, no regressions.
>
> > This is ok if you move the testcase from g++.dg/tree-ssa to g++.dg/torture
> > (and remove the dg-options then).
>
> Thanks, here's what I've just installed in the trunk.

The extra checking triggers PR33735.

Richard.

Re: [vtab (and trunk?)] fix fallout from SSA-coalescing user variables

[Alexandre Oliva]

On Oct 11, 2007, "Richard Guenther" <richard.guenther@gmail.com> wrote:

> The extra checking triggers PR33735.

Thanks.  This is an interesting testcase.  It is a variant of the
problem H-P had reported, but I hadn't realized there was a lingering
bug then.

The problem is that an exception thrown during stack unwinding is
supposed to call terminate(), but AFAICT, when we inline the dtor of
temporaries (other than the exception object) in the throw expression
itself, calls that are brought in as part of the destructor get EH
edges as if they were before the throw expression, escaping the
requirement to call terminate in case they throw.

Now, since there aren't edges from the original dtor call to enclosing
catch or clean-up points, when we look for such edges at the time of
adjusting the PHI nodes, we don't find them.  Oops.

I'll look more into it after getting some sleep.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: [vtab (and trunk?)] fix fallout from SSA-coalescing user variables

[Alexandre Oliva]

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

On Oct 12, 2007, Alexandre Oliva <aoliva@redhat.com> wrote:

> On Oct 11, 2007, "Richard Guenther" <richard.guenther@gmail.com> wrote:
>> The extra checking triggers PR33735.

> Thanks.  This is an interesting testcase.  It is a variant of the
> problem H-P had reported, but I hadn't realized there was a lingering
> bug then.

So the patch below reverts the work around for H-P's testcase, and
fixes the problem explained below.

> The problem is that an exception thrown during stack unwinding is
> supposed to call terminate(), but AFAICT, when we inline the dtor of
> temporaries (other than the exception object) in the throw expression
> itself, calls that are brought in as part of the destructor get EH
> edges as if they were before the throw expression, escaping the
> requirement to call terminate in case they throw.

> Now, since there aren't edges from the original dtor call to enclosing
> catch or clean-up points, when we look for such edges at the time of
> adjusting the PHI nodes, we don't find them.  Oops.

The patch below recognizes an ERT_ALLOWED_EXCEPTIONS EH region with an
empty type list and prevents the enclosing try block from being
regarded as reachable from within.  With it, both testcases compile
correctly.

libstdc++ builds with it; starting bootstrap and regtesting on
x86_64-linux-gnu right now.  Ok to install?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-pr33572-pr33735.patch --]
[-- Type: text/x-patch, Size: 2396 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR tree-optimization/33735
	PR tree-optimization/33572
	* tree-inline.c (update_ssa_across_abnormal_edges): Revert
	2007-10-09's change.
	* except.c (duplicate_eh_regions): Don't look for prev_try
	beyond ERT_ALLOWED_EXCEPTIONS with an empty list.
	
for  gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR tree-optimization/33735
	* g++.dg/torture/pr33735.C: New.

Index: gcc/tree-inline.c
===================================================================
--- gcc/tree-inline.c.orig	2007-10-13 04:38:25.000000000 -0300
+++ gcc/tree-inline.c	2007-10-13 05:47:02.000000000 -0300
@@ -1055,8 +1055,7 @@ update_ssa_across_abnormal_edges (basic_
 	      }
 
 	    re = find_edge (ret_bb, e->dest);
-	    if (!re)
-	      continue;
+	    gcc_assert (re);
 	    gcc_assert ((re->flags & (EDGE_EH | EDGE_ABNORMAL))
 			== (e->flags & (EDGE_EH | EDGE_ABNORMAL)));
 
Index: gcc/except.c
===================================================================
--- gcc/except.c.orig	2007-09-28 00:02:42.000000000 -0300
+++ gcc/except.c	2007-10-13 05:24:54.000000000 -0300
@@ -1005,7 +1005,9 @@ duplicate_eh_regions (struct function *i
     for (prev_try = VEC_index (eh_region, cfun->eh->region_array, outer_region);
          prev_try && prev_try->type != ERT_TRY;
 	 prev_try = prev_try->outer)
-      if (prev_try->type == ERT_MUST_NOT_THROW)
+      if (prev_try->type == ERT_MUST_NOT_THROW
+	  || (prev_try->type == ERT_ALLOWED_EXCEPTIONS
+	      && !prev_try->u.allowed.type_list))
 	{
 	  prev_try = NULL;
 	  break;
Index: gcc/testsuite/g++.dg/torture/pr33735.C
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/g++.dg/torture/pr33735.C	2007-10-13 05:44:39.000000000 -0300
@@ -0,0 +1,21 @@
+// { dg-do compile }
+#include <string>
+typedef struct _ts { } PyThreadState;
+PyThreadState * Py_NewInterpreter(void);
+void Py_EndInterpreter(PyThreadState *);
+class ApplicationError {
+public:
+    ApplicationError(std::string errormsg) : errormsg(errormsg)  { }
+    std::string errormsg;
+};
+void run()
+{
+    PyThreadState *py_state=__null;
+    try {
+        if (!(py_state=Py_NewInterpreter()))
+            throw ApplicationError("error");
+    }
+    catch(ApplicationError e) {
+        Py_EndInterpreter(py_state);
+    }
+}

[-- Attachment #3: Type: text/plain, Size: 249 bytes --]


-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: [vtab (and trunk?)] fix fallout from SSA-coalescing user variables

[Alexandre Oliva]

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

On Oct 12, 2007, Alexandre Oliva <aoliva@redhat.com> wrote:

> On Oct 11, 2007, "Richard Guenther" <richard.guenther@gmail.com> wrote:
>> The extra checking triggers PR33735.

> Thanks.  This is an interesting testcase.  It is a variant of the
> problem H-P had reported, but I hadn't realized there was a lingering
> bug then.

So the patch below reverts the work around for H-P's testcase, and
fixes the problem explained below.

> The problem is that an exception thrown during stack unwinding is
> supposed to call terminate(), but AFAICT, when we inline the dtor of
> temporaries (other than the exception object) in the throw expression
> itself, calls that are brought in as part of the destructor get EH
> edges as if they were before the throw expression, escaping the
> requirement to call terminate in case they throw.

> Now, since there aren't edges from the original dtor call to enclosing
> catch or clean-up points, when we look for such edges at the time of
> adjusting the PHI nodes, we don't find them.  Oops.

The patch below recognizes an ERT_ALLOWED_EXCEPTIONS EH region with an
empty type list and prevents the enclosing try block from being
regarded as reachable from within.  With it, both testcases compile
correctly.

libstdc++ builds with it; starting bootstrap and regtesting on
x86_64-linux-gnu right now.  Ok to install?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-pr33572-pr33735.patch --]
[-- Type: text/x-patch, Size: 2396 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR tree-optimization/33735
	PR tree-optimization/33572
	* tree-inline.c (update_ssa_across_abnormal_edges): Revert
	2007-10-09's change.
	* except.c (duplicate_eh_regions): Don't look for prev_try
	beyond ERT_ALLOWED_EXCEPTIONS with an empty list.
	
for  gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR tree-optimization/33735
	* g++.dg/torture/pr33735.C: New.

Index: gcc/tree-inline.c
===================================================================
--- gcc/tree-inline.c.orig	2007-10-13 04:38:25.000000000 -0300
+++ gcc/tree-inline.c	2007-10-13 05:47:02.000000000 -0300
@@ -1055,8 +1055,7 @@ update_ssa_across_abnormal_edges (basic_
 	      }
 
 	    re = find_edge (ret_bb, e->dest);
-	    if (!re)
-	      continue;
+	    gcc_assert (re);
 	    gcc_assert ((re->flags & (EDGE_EH | EDGE_ABNORMAL))
 			== (e->flags & (EDGE_EH | EDGE_ABNORMAL)));
 
Index: gcc/except.c
===================================================================
--- gcc/except.c.orig	2007-09-28 00:02:42.000000000 -0300
+++ gcc/except.c	2007-10-13 05:24:54.000000000 -0300
@@ -1005,7 +1005,9 @@ duplicate_eh_regions (struct function *i
     for (prev_try = VEC_index (eh_region, cfun->eh->region_array, outer_region);
          prev_try && prev_try->type != ERT_TRY;
 	 prev_try = prev_try->outer)
-      if (prev_try->type == ERT_MUST_NOT_THROW)
+      if (prev_try->type == ERT_MUST_NOT_THROW
+	  || (prev_try->type == ERT_ALLOWED_EXCEPTIONS
+	      && !prev_try->u.allowed.type_list))
 	{
 	  prev_try = NULL;
 	  break;
Index: gcc/testsuite/g++.dg/torture/pr33735.C
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/g++.dg/torture/pr33735.C	2007-10-13 05:44:39.000000000 -0300
@@ -0,0 +1,21 @@
+// { dg-do compile }
+#include <string>
+typedef struct _ts { } PyThreadState;
+PyThreadState * Py_NewInterpreter(void);
+void Py_EndInterpreter(PyThreadState *);
+class ApplicationError {
+public:
+    ApplicationError(std::string errormsg) : errormsg(errormsg)  { }
+    std::string errormsg;
+};
+void run()
+{
+    PyThreadState *py_state=__null;
+    try {
+        if (!(py_state=Py_NewInterpreter()))
+            throw ApplicationError("error");
+    }
+    catch(ApplicationError e) {
+        Py_EndInterpreter(py_state);
+    }
+}

[-- Attachment #3: Type: text/plain, Size: 249 bytes --]


-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: [vtab (and trunk?)] fix fallout from SSA-coalescing user variables

[Richard Guenther]

On 10/13/07, Alexandre Oliva <aoliva@redhat.com> wrote:
> > On Oct 11, 2007, "Richard Guenther" <richard.guenther@gmail.com> wrote:
> >> The extra checking triggers PR33735.
>
> > Thanks.  This is an interesting testcase.  It is a variant of the
> > problem H-P had reported, but I hadn't realized there was a lingering
> > bug then.
>
> So the patch below reverts the work around for H-P's testcase, and
> fixes the problem explained below.
>
> > The problem is that an exception thrown during stack unwinding is
> > supposed to call terminate(), but AFAICT, when we inline the dtor of
> > temporaries (other than the exception object) in the throw expression
> > itself, calls that are brought in as part of the destructor get EH
> > edges as if they were before the throw expression, escaping the
> > requirement to call terminate in case they throw.
>
> > Now, since there aren't edges from the original dtor call to enclosing
> > catch or clean-up points, when we look for such edges at the time of
> > adjusting the PHI nodes, we don't find them.  Oops.
>
> The patch below recognizes an ERT_ALLOWED_EXCEPTIONS EH region with an
> empty type list and prevents the enclosing try block from being
> regarded as reachable from within.  With it, both testcases compile
> correctly.
>
> libstdc++ builds with it; starting bootstrap and regtesting on
> x86_64-linux-gnu right now.  Ok to install?

This is ok.

Thanks,
Richard.