Patchwork [CFT,try,9] Emulated tls rewrite

login
register
mail settings
Submitter Richard Henderson
Date July 21, 2010, 7:38 p.m.
Message ID <4C474CB6.9080303@redhat.com>
Download mbox | patch
Permalink /patch/59491/
State New
Headers show

Comments

Richard Henderson - July 21, 2010, 7:38 p.m.
On 07/21/2010 09:32 AM, Richard Henderson wrote:
> On 07/21/2010 01:23 AM, Richard Guenther wrote:
>> Couldn't you do gsi_insert_on_edge_immediate?
> 
> I could.  It doesn't really help though, since the location at which
> the call statement and the call edge are created is shared between
> the normal statements and the phi statements.  It gets ugly either way.

As discussed on IRC, what do you think of this incremental patch?

I've tested it on tls.exp and see no failures.  I'll start a proper
bootstrap and test in a moment to see that there are no other 
unexpected consequences.



r~
Jakub Jelinek - July 21, 2010, 7:46 p.m.
On Wed, Jul 21, 2010 at 12:38:30PM -0700, Richard Henderson wrote:
> On 07/21/2010 09:32 AM, Richard Henderson wrote:
> > On 07/21/2010 01:23 AM, Richard Guenther wrote:
> >> Couldn't you do gsi_insert_on_edge_immediate?
> > 
> > I could.  It doesn't really help though, since the location at which
> > the call statement and the call edge are created is shared between
> > the normal statements and the phi statements.  It gets ugly either way.
> 
> As discussed on IRC, what do you think of this incremental patch?
> 
> I've tested it on tls.exp and see no failures.  I'll start a proper
> bootstrap and test in a moment to see that there are no other 
> unexpected consequences.

> --- a/gcc/gimple-iterator.c
> +++ b/gcc/gimple-iterator.c
> @@ -65,6 +65,25 @@ update_bb_for_stmts (gimple_seq_node first, basic_block bb)
>      gimple_set_bb (n->stmt, bb);
>  }
>  
> +/* Set the frequencies for the cgraph_edges for each of the calls
> +   starting at FIRST for their new position within BB.  */
> +
> +static void
> +update_call_edge_frequencies (gimple_seq_node first, basic_block bb)
> +{
> +  int bb_freq = compute_call_stmt_bb_frequency (current_function_decl, bb);
> +  struct cgraph_node *cfun_node = cgraph_node (current_function_decl);

Aren't the above two calls expensive enough that it would be better to
only call them after seeing first is_gimple_call?
Many bbs don't contain any calls...

> +  gimple_seq_node n;
> +
> +  for (n = first; n ; n = n->next)
> +    if (is_gimple_call (n->stmt))
> +      {
> +	struct cgraph_edge *e = cgraph_edge (cfun_node, n->stmt);
> +	if (e != NULL)
> +	  e->frequency = bb_freq;
> +      }
> +}
> +

	Jakub
Richard Henderson - July 21, 2010, 7:54 p.m.
On 07/21/2010 12:46 PM, Jakub Jelinek wrote:
>> +  int bb_freq = compute_call_stmt_bb_frequency (current_function_decl, bb);
>> +  struct cgraph_node *cfun_node = cgraph_node (current_function_decl);
> 
> Aren't the above two calls expensive enough that it would be better to
> only call them after seeing first is_gimple_call?
> Many bbs don't contain any calls...

You're probably right.  Consider the patch adjusted.


r~
Jack Howarth - July 22, 2010, 2:32 a.m.
On Wed, Jul 21, 2010 at 12:38:30PM -0700, Richard Henderson wrote:
> On 07/21/2010 09:32 AM, Richard Henderson wrote:
> > On 07/21/2010 01:23 AM, Richard Guenther wrote:
> >> Couldn't you do gsi_insert_on_edge_immediate?
> > 
> > I could.  It doesn't really help though, since the location@which
> > the call statement and the call edge are created is shared between
> > the normal statements and the phi statements.  It gets ugly either way.
> 
> As discussed on IRC, what do you think of this incremental patch?
> 
> I've tested it on tls.exp and see no failures.  I'll start a proper
> bootstrap and test in a moment to see that there are no other 
> unexpected consequences.
> 
> 
> 
> r~
> 

Richard,
   I am still seeing one additional failure in the gfortran testsuite
(with the expanded tls test set) under your emutls patch that wasn't
present with Iain's original version. For the gfortran.dg/gomp/appendix-a/a.22.6.f90
testcase with...

! { dg-require-effective-target tls }

I get the failure...

Executing on host: /sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/gcc/testsuite/gfortran/../../gfortran -B/sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/gcc/testsuite/gfortran/../../ /sw/src/fink.build/gcc46-4.6.0-1000/gcc-4.6-20100721/gcc/testsuite/gfortran.dg/gomp/appendix-a/a.22.6.f90   -O   -fopenmp -S  -o a.22.6.s    (timeout = 300)
/sw/src/fink.build/gcc46-4.6.0-1000/gcc-4.6-20100721/gcc/testsuite/gfortran.dg/gomp/appendix-a/a.22.6.f90:4:0: sorry, unimplemented: thread-local COMMON data not implemented^M
compiler exited with status 1
output is:
/sw/src/fink.build/gcc46-4.6.0-1000/gcc-4.6-20100721/gcc/testsuite/gfortran.dg/gomp/appendix-a/a.22.6.f90:4:0: sorry, unimplemented: thread-local COMMON data not implemented^M

FAIL: gfortran.dg/gomp/appendix-a/a.22.6.f90  -O  (test for excess errors)
Excess errors:
/sw/src/fink.build/gcc46-4.6.0-1000/gcc-4.6-20100721/gcc/testsuite/gfortran.dg/gomp/appendix-a/a.22.6.f90:4:0: sorry, unimplemented: thread-local COMMON data not implemented


Any idea why your version (with the emutls code as a pass) fails gfortran.dg/gomp/appendix-a/a.22.6.f90
and Iain's version didn't?
        Jack
IainS - July 22, 2010, 10:12 a.m.
Hi Jack, Richard,

On 22 Jul 2010, at 03:32, Jack Howarth wrote:
>   I am still seeing one additional failure in the gfortran testsuite
> (with the expanded tls test set) under your emutls patch

8b or...
... 8b + http://gcc.gnu.org/ml/gcc-patches/2010-07/msg01671.html ?

> Executing on host: /sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/ 
> gcc/testsuite/gfortran/../../gfortran -B/sw/src/fink.build/ 
> gcc46-4.6.0-1000/darwin_objdir/gcc/testsuite/gfortran/../../ /sw/src/ 
> fink.build/gcc46-4.6.0-1000/gcc-4.6-20100721/gcc/testsuite/ 
> gfortran.dg/gomp/appendix-a/a.22.6.f90   -O   -fopenmp -S  -o a. 
> 22.6.s    (timeout = 300)
> /sw/src/fink.build/gcc46-4.6.0-1000/gcc-4.6-20100721/gcc/testsuite/ 
> gfortran.dg/gomp/appendix-a/a.22.6.f90:4:0: sorry, unimplemented:  
> thread-local COMMON data not implemented^M

I do _not_ see this on either i686-darwin9 or x86_64-darwin10

with r162403 + emutls-8b.

.. . sorry, I haven't tried the increment, been busy with bootstrap  
issues..
...  (although the increment  looks to be still in flux)..

Iain
Richard Henderson - July 22, 2010, 4:12 p.m.
On 07/21/2010 07:32 PM, Jack Howarth wrote:
> /sw/src/fink.build/gcc46-4.6.0-1000/gcc-4.6-20100721/gcc/testsuite/gfortran.dg/gomp/appendix-a/a.22.6.f90:4:0:
> sorry, unimplemented: thread-local COMMON data not implemented
> 
> Any idea why your version (with the emutls code as a pass) fails
> gfortran.dg/gomp/appendix-a/a.22.6.f90 and Iain's version didn't?

I have absolutely no idea how you can get that message.

Suppose that a TLS variable did slip past the IPA pass and didn't get
lowered.  Then you *should* have failed the gcc_checking_assert right
at the top of assemble_variable.



r~
Jack Howarth - July 22, 2010, 4:14 p.m.
On Thu, Jul 22, 2010 at 11:12:53AM +0100, IainS wrote:
> Hi Jack, Richard,
>
> On 22 Jul 2010, at 03:32, Jack Howarth wrote:
>>   I am still seeing one additional failure in the gfortran testsuite
>> (with the expanded tls test set) under your emutls patch
>
> 8b or...
> ... 8b + http://gcc.gnu.org/ml/gcc-patches/2010-07/msg01671.html ?
>
>> Executing on host: /sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/ 
>> gcc/testsuite/gfortran/../../gfortran -B/sw/src/fink.build/ 
>> gcc46-4.6.0-1000/darwin_objdir/gcc/testsuite/gfortran/../../ /sw/src/ 
>> fink.build/gcc46-4.6.0-1000/gcc-4.6-20100721/gcc/testsuite/ 
>> gfortran.dg/gomp/appendix-a/a.22.6.f90   -O   -fopenmp -S  -o a.22.6.s  
>>   (timeout = 300)
>> /sw/src/fink.build/gcc46-4.6.0-1000/gcc-4.6-20100721/gcc/testsuite/ 
>> gfortran.dg/gomp/appendix-a/a.22.6.f90:4:0: sorry, unimplemented:  
>> thread-local COMMON data not implemented^M
>
> I do _not_ see this on either i686-darwin9 or x86_64-darwin10

Iain,
   Apparently this must have been fixed with http://gcc.gnu.org/ml/gcc-patches/2010-07/msg01671.html.
I don't see the problem with that and http://gcc.gnu.org/ml/gcc-patches/2010-07/msg01528.html
applied to r162414 on x86_64-apple-darwin10.
                     Jack
>
> with r162403 + emutls-8b.
>
> .. . sorry, I haven't tried the increment, been busy with bootstrap  
> issues..
> ...  (although the increment  looks to be still in flux)..
>
> Iain

Patch

diff --git a/gcc/gimple-iterator.c b/gcc/gimple-iterator.c
index 1402c0d..291e4d9 100644
--- a/gcc/gimple-iterator.c
+++ b/gcc/gimple-iterator.c
@@ -65,6 +65,25 @@  update_bb_for_stmts (gimple_seq_node first, basic_block bb)
     gimple_set_bb (n->stmt, bb);
 }
 
+/* Set the frequencies for the cgraph_edges for each of the calls
+   starting at FIRST for their new position within BB.  */
+
+static void
+update_call_edge_frequencies (gimple_seq_node first, basic_block bb)
+{
+  int bb_freq = compute_call_stmt_bb_frequency (current_function_decl, bb);
+  struct cgraph_node *cfun_node = cgraph_node (current_function_decl);
+  gimple_seq_node n;
+
+  for (n = first; n ; n = n->next)
+    if (is_gimple_call (n->stmt))
+      {
+	struct cgraph_edge *e = cgraph_edge (cfun_node, n->stmt);
+	if (e != NULL)
+	  e->frequency = bb_freq;
+      }
+}
+
 
 /* Insert the sequence delimited by nodes FIRST and LAST before
    iterator I.  M specifies how to update iterator I after insertion
@@ -696,11 +715,19 @@  basic_block
 gsi_insert_on_edge_immediate (edge e, gimple stmt)
 {
   gimple_stmt_iterator gsi;
+  struct gimple_seq_node_d node;
   basic_block new_bb = NULL;
+  bool ins_after;
 
   gcc_assert (!PENDING_STMT (e));
 
-  if (gimple_find_edge_insert_loc (e, &gsi, &new_bb))
+  ins_after = gimple_find_edge_insert_loc (e, &gsi, &new_bb);
+
+  node.stmt = stmt;
+  node.prev = node.next = NULL;
+  update_call_edge_frequencies (&node, gsi.bb);
+
+  if (ins_after)
     gsi_insert_after (&gsi, stmt, GSI_NEW_STMT);
   else
     gsi_insert_before (&gsi, stmt, GSI_NEW_STMT);
@@ -716,10 +743,14 @@  gsi_insert_seq_on_edge_immediate (edge e, gimple_seq stmts)
 {
   gimple_stmt_iterator gsi;
   basic_block new_bb = NULL;
+  bool ins_after;
 
   gcc_assert (!PENDING_STMT (e));
 
-  if (gimple_find_edge_insert_loc (e, &gsi, &new_bb))
+  ins_after = gimple_find_edge_insert_loc (e, &gsi, &new_bb);
+  update_call_edge_frequencies (gimple_seq_first (stmts), gsi.bb);
+
+  if (ins_after)
     gsi_insert_seq_after (&gsi, stmts, GSI_NEW_STMT);
   else
     gsi_insert_seq_before (&gsi, stmts, GSI_NEW_STMT);
@@ -744,7 +775,6 @@  gsi_commit_edge_inserts (void)
       gsi_commit_one_edge_insert (e, NULL);
 }
 
-
 /* Commit insertions pending at edge E. If a new block is created, set NEW_BB
    to this block, otherwise set it to NULL.  */
 
@@ -758,10 +788,14 @@  gsi_commit_one_edge_insert (edge e, basic_block *new_bb)
     {
       gimple_stmt_iterator gsi;
       gimple_seq seq = PENDING_STMT (e);
+      bool ins_after;
 
       PENDING_STMT (e) = NULL;
 
-      if (gimple_find_edge_insert_loc (e, &gsi, new_bb))
+      ins_after = gimple_find_edge_insert_loc (e, &gsi, new_bb);
+      update_call_edge_frequencies (gimple_seq_first (seq), gsi.bb);
+
+      if (ins_after)
 	gsi_insert_seq_after (&gsi, seq, GSI_NEW_STMT);
       else
 	gsi_insert_seq_before (&gsi, seq, GSI_NEW_STMT);
diff --git a/gcc/tree-emutls.c b/gcc/tree-emutls.c
index 8b7c7d8..17f97be 100644
--- a/gcc/tree-emutls.c
+++ b/gcc/tree-emutls.c
@@ -596,61 +596,6 @@  clear_access_vars (void)
           VEC_length (tree, access_vars) * sizeof(tree));
 }
 
-/* Compute the frequency of instructions to be insertted on an edge E.
-
-   Mirror a portion of the logic from gimple_find_edge_insert_loc to
-   do this.  Determine if statements inserted on E will be insertted
-   into the predecessor block.  We must choose the correct block,
-   and thus frequency, in order to pass verify_cgraph.  */
-
-static int
-compute_edge_bb_frequency (edge e)
-{
-  basic_block src = e->src;
-
-  /* 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.  */
-  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))
-	goto return_pred;
-
-      /* If the last stmt does not end the block, we insert after.  */
-      last = gsi_stmt (gsi);
-      if (!stmt_ends_bb_p (last))
-	goto return_pred;
-
-      /* If the last stmt is a trivial control, we insert before.  */
-      switch (gimple_code (last))
-	{
-	case GIMPLE_RETURN:
-	case GIMPLE_RESX:
-	  goto return_pred;
-	default:
-	  break;
-	}
-    }
-
-  /* We will be splitting the edge and insertting there.  */
-  return EDGE_FREQUENCY (e);
-
-  /* If we get here, we will not be splitting the edge and instead
-     insertting any code into the predecessor block.  */
- return_pred:
-  return compute_call_stmt_bb_frequency (current_function_decl, e->src);
-}
-
 /* Lower the entire function NODE.  */
 
 static void
@@ -677,13 +622,15 @@  lower_emutls_function_body (struct cgraph_node *node)
 	 PHI argument for that edge.  */
       if (!gimple_seq_empty_p (phi_nodes (d.bb)))
 	{
+	  /* The calls will be inserted on the edges, and the frequencies
+	     will be computed during the commit process.  */
+	  d.bb_freq = 0;
+
 	  nedge = EDGE_COUNT (d.bb->preds);
 	  for (i = 0; i < nedge; ++i)
 	    {
 	      edge e = EDGE_PRED (d.bb, i);
 
-	      d.bb_freq = compute_edge_bb_frequency (e);
-
 	      /* We can re-use any SSA_NAME created on this edge.  */
 	      clear_access_vars ();
 	      d.seq = NULL;