Patchwork [CFT,try,7] Emulated tls rewrite

login
register
mail settings
Submitter Richard Henderson
Date July 15, 2010, 3:50 p.m.
Message ID <4C3F2E42.4090605@redhat.com>
Download mbox | patch
Permalink /patch/58989/
State New
Headers show

Comments

Richard Henderson - July 15, 2010, 3:50 p.m.
On 07/15/2010 12:59 AM, IainS wrote:
> unfortunately, although it applies cleanly, it doesn't bootstrap for me
> (against either 162180 or 162193).
> (fails in building libgomp/parallel.c with 'caller edge frequency 1000
> does not match BB frequency of 61')...
> ... I will look a little more - but this is uncharted territory for me ;)

Incremental patch follows.


r~
IainS - July 15, 2010, 8:28 p.m.
On 15 Jul 2010, at 16:50, Richard Henderson wrote:

> On 07/15/2010 12:59 AM, IainS wrote:
>> unfortunately, although it applies cleanly, it doesn't bootstrap  
>> for me
>> (against either 162180 or 162193).
>> (fails in building libgomp/parallel.c with 'caller edge frequency  
>> 1000
>> does not match BB frequency of 61')...
>> ... I will look a little more - but this is uncharted territory for  
>> me ;)
>
> Incremental patch follows.

I applied emutls-7 + the increment to 162222.

It is clean on cris-elf "tls.exp=*"  (emutls target with alias, but no  
libgomp)

also on i686-apple-darwin9 (emutls target without alias but with  
libgomp)

These were done with all the test-suite additions mentioned below.

----
I believe this clears (at least):
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44132
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44137 (together with the  
ObjC LTO patch already applied)
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44276

-----
Note1:

an updated version of this could be applied:

http://gcc.gnu.org/ml/gcc-patches/2010-05/msg01738.html

In fact, it could be applied anyway, the current emutls implementation  
works with these tests.

updated patch for that attached - the changelog should still stand  
(+/- minor adjustments).

----
Note2:

The cse test I sent you only works on darwin (m32) when the branch  
island patch is also applied.
(although, of course, it should be valid for other targets and darwin  
at m64).

---
Note 3:
I've got a set of tls tests for ObjC++, will post those separately (I  
want to trim them a bit).

----
Note 4:
I'll also un-xfail the ObjC tls tests when this goes in.

=====

Thanks very much for working on this....
Iain

==== extended tls test-suite vs 162229.
Jack Howarth - July 19, 2010, 1:18 p.m.
On Thu, Jul 15, 2010 at 08:50:26AM -0700, Richard Henderson wrote:
> On 07/15/2010 12:59 AM, IainS wrote:
> > unfortunately, although it applies cleanly, it doesn't bootstrap for me
> > (against either 162180 or 162193).
> > (fails in building libgomp/parallel.c with 'caller edge frequency 1000
> > does not match BB frequency of 61')...
> > ... I will look a little more - but this is uncharted territory for me ;)
> 
> Incremental patch follows.
> 
> 
> r~

Richard,
   Do you plan to repost the emutls_7 patch combined with this patch
and corrected for bit rot? Also, is there anything left to be done
with the emutls lto patch ? If not, perhaps you can just commit the
current version. Thanks in advance.
           Jack

> diff --git a/gcc/tree-emutls.c b/gcc/tree-emutls.c
> index f7eb4cd..275e16b 100644
> --- a/gcc/tree-emutls.c
> +++ b/gcc/tree-emutls.c
> @@ -580,6 +580,54 @@ lower_emutls_phi_arg (gimple phi, unsigned int i, struct lower_emutls_data *d)
>      }
>  }
>  
> +/* Mirror a portion of the logic from gimple_find_edge_insert_loc.
> +   Determine if statements inserted on E will be insertted into the
> +   predecessor block.  We *must* match gimple_find_edge_insert_loc
> +   in order to pass verify_cgraph. 
> +
> +   If the destination has one predecessor and no PHI nodes, insert
> +   there... Except that we *know* it has PHI nodes or else there's
> +   nothing for us to do.  Skip this step.
> +
> +   If the source has one successor, the edge is not abnormal and the
> +   last statement does not end a basic block, insert there.  */
> +
> +static bool
> +will_insert_in_edge_src (edge e)
> +{
> +  basic_block src = e->src;
> +
> +  if ((e->flags & EDGE_ABNORMAL) == 0
> +      && src != ENTRY_BLOCK_PTR
> +      && single_succ_p (src))
> +    {
> +      gimple_stmt_iterator gsi;
> +      gimple last;
> +
> +      /* If the block is empty, of course we use it.  */
> +      gsi = gsi_last_bb (src);
> +      if (gsi_end_p (gsi))
> +	return true;
> +
> +      /* If the last stmt does not end the block, we insert after.  */
> +      last = gsi_stmt (gsi);
> +      if (!stmt_ends_bb_p (last))
> +	return true;
> +
> +      /* If the last stmt is a trivial control, we insert before.  */
> +      switch (gimple_code (last))
> +	{
> +	case GIMPLE_RETURN:
> +	case GIMPLE_RESX:
> +	  return true;
> +	default:
> +	  break;
> +	}
> +    }
> +
> +  return false;
> +}
> +
>  /* Lower the entire function NODE.  */
>  
>  static void
> @@ -605,34 +653,41 @@ lower_emutls_function_body (struct cgraph_node *node)
>        gimple_stmt_iterator gsi;
>        unsigned int i, nedge;
>  
> -      /* ??? This is the frequency of the new block that might be created by
> -	 split_edge.  One might have thought EDGE_FREQUENCY.  However, this
> -         *must* match split_edge, or we'll fail verify_cgraph.  */
> -      d.bb_freq = CGRAPH_FREQ_BASE;
> -
>        /* Lower each of the PHI nodes of the block, as we may have 
>  	 propagated &tlsvar into a PHI argument.  These loops are
>  	 arranged so that we process each edge at once, and each
>  	 PHI argument for that edge.  */
> -
> -      nedge = EDGE_COUNT (d.bb->preds);
> -      for (i = 0; i < nedge; ++i)
> +      if (!gimple_seq_empty_p (phi_nodes (d.bb)))
>  	{
> -	  edge e = EDGE_PRED (d.bb, i);
> -
> -          /* We can re-use any SSA_NAME created on this edge.  */
> -          memset (VEC_address (tree, d.accessed), 0, n_tls * sizeof(tree));
> -	  d.seq = NULL;
> -
> -	  for (gsi = gsi_start_phis (d.bb); !gsi_end_p (gsi); gsi_next (&gsi))
> -	    lower_emutls_phi_arg (gsi_stmt (gsi), i, &d);
> -
> -	  /* Insert all statements generated by all phi nodes for this
> -	     particular edge all at once.  */
> -	  if (d.seq)
> +	  nedge = EDGE_COUNT (d.bb->preds);
> +	  for (i = 0; i < nedge; ++i)
>  	    {
> -	      gsi_insert_seq_on_edge (e, d.seq);
> -	      any_edge_inserts = true;
> +	      edge e = EDGE_PRED (d.bb, i);
> +
> +	      /* Choose the correct frequency for stmts to be
> +		 insertted on this edge.  */
> +	      if (will_insert_in_edge_src (e))
> +		d.bb_freq = (compute_call_stmt_bb_frequency
> +			     (current_function_decl, e->src));
> +	      else
> +		d.bb_freq = EDGE_FREQUENCY (e);
> +
> +	      /* We can re-use any SSA_NAME created on this edge.  */
> +	      memset (VEC_address (tree, d.accessed), 0, n_tls * sizeof(tree));
> +	      d.seq = NULL;
> +
> +	      for (gsi = gsi_start_phis (d.bb);
> +		   !gsi_end_p (gsi);
> +		   gsi_next (&gsi))
> +		lower_emutls_phi_arg (gsi_stmt (gsi), i, &d);
> +
> +	      /* Insert all statements generated by all phi nodes for this
> +		 particular edge all at once.  */
> +	      if (d.seq)
> +		{
> +		  gsi_insert_seq_on_edge (e, d.seq);
> +		  any_edge_inserts = true;
> +		}
>  	    }
>  	}
>

Patch

diff --git a/gcc/tree-emutls.c b/gcc/tree-emutls.c
index f7eb4cd..275e16b 100644
--- a/gcc/tree-emutls.c
+++ b/gcc/tree-emutls.c
@@ -580,6 +580,54 @@  lower_emutls_phi_arg (gimple phi, unsigned int i, struct lower_emutls_data *d)
     }
 }
 
+/* Mirror a portion of the logic from gimple_find_edge_insert_loc.
+   Determine if statements inserted on E will be insertted into the
+   predecessor block.  We *must* match gimple_find_edge_insert_loc
+   in order to pass verify_cgraph. 
+
+   If the destination has one predecessor and no PHI nodes, insert
+   there... Except that we *know* it has PHI nodes or else there's
+   nothing for us to do.  Skip this step.
+
+   If the source has one successor, the edge is not abnormal and the
+   last statement does not end a basic block, insert there.  */
+
+static bool
+will_insert_in_edge_src (edge e)
+{
+  basic_block src = e->src;
+
+  if ((e->flags & EDGE_ABNORMAL) == 0
+      && src != ENTRY_BLOCK_PTR
+      && single_succ_p (src))
+    {
+      gimple_stmt_iterator gsi;
+      gimple last;
+
+      /* If the block is empty, of course we use it.  */
+      gsi = gsi_last_bb (src);
+      if (gsi_end_p (gsi))
+	return true;
+
+      /* If the last stmt does not end the block, we insert after.  */
+      last = gsi_stmt (gsi);
+      if (!stmt_ends_bb_p (last))
+	return true;
+
+      /* If the last stmt is a trivial control, we insert before.  */
+      switch (gimple_code (last))
+	{
+	case GIMPLE_RETURN:
+	case GIMPLE_RESX:
+	  return true;
+	default:
+	  break;
+	}
+    }
+
+  return false;
+}
+
 /* Lower the entire function NODE.  */
 
 static void
@@ -605,34 +653,41 @@  lower_emutls_function_body (struct cgraph_node *node)
       gimple_stmt_iterator gsi;
       unsigned int i, nedge;
 
-      /* ??? This is the frequency of the new block that might be created by
-	 split_edge.  One might have thought EDGE_FREQUENCY.  However, this
-         *must* match split_edge, or we'll fail verify_cgraph.  */
-      d.bb_freq = CGRAPH_FREQ_BASE;
-
       /* Lower each of the PHI nodes of the block, as we may have 
 	 propagated &tlsvar into a PHI argument.  These loops are
 	 arranged so that we process each edge at once, and each
 	 PHI argument for that edge.  */
-
-      nedge = EDGE_COUNT (d.bb->preds);
-      for (i = 0; i < nedge; ++i)
+      if (!gimple_seq_empty_p (phi_nodes (d.bb)))
 	{
-	  edge e = EDGE_PRED (d.bb, i);
-
-          /* We can re-use any SSA_NAME created on this edge.  */
-          memset (VEC_address (tree, d.accessed), 0, n_tls * sizeof(tree));
-	  d.seq = NULL;
-
-	  for (gsi = gsi_start_phis (d.bb); !gsi_end_p (gsi); gsi_next (&gsi))
-	    lower_emutls_phi_arg (gsi_stmt (gsi), i, &d);
-
-	  /* Insert all statements generated by all phi nodes for this
-	     particular edge all at once.  */
-	  if (d.seq)
+	  nedge = EDGE_COUNT (d.bb->preds);
+	  for (i = 0; i < nedge; ++i)
 	    {
-	      gsi_insert_seq_on_edge (e, d.seq);
-	      any_edge_inserts = true;
+	      edge e = EDGE_PRED (d.bb, i);
+
+	      /* Choose the correct frequency for stmts to be
+		 insertted on this edge.  */
+	      if (will_insert_in_edge_src (e))
+		d.bb_freq = (compute_call_stmt_bb_frequency
+			     (current_function_decl, e->src));
+	      else
+		d.bb_freq = EDGE_FREQUENCY (e);
+
+	      /* We can re-use any SSA_NAME created on this edge.  */
+	      memset (VEC_address (tree, d.accessed), 0, n_tls * sizeof(tree));
+	      d.seq = NULL;
+
+	      for (gsi = gsi_start_phis (d.bb);
+		   !gsi_end_p (gsi);
+		   gsi_next (&gsi))
+		lower_emutls_phi_arg (gsi_stmt (gsi), i, &d);
+
+	      /* Insert all statements generated by all phi nodes for this
+		 particular edge all at once.  */
+	      if (d.seq)
+		{
+		  gsi_insert_seq_on_edge (e, d.seq);
+		  any_edge_inserts = true;
+		}
 	    }
 	}