| Message ID | 4C474CB6.9080303@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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~
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
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
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~
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
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;