Message ID | 20110202140748.GM30899@tyan-ft48-01.lab.bos.redhat.com |
---|---|
State | New |
Headers | show |
On Wed, Feb 2, 2011 at 3:07 PM, Jakub Jelinek <jakub@redhat.com> wrote: > Hi! > > This patch fixes a target attribute handling bug. set_cfun when changing > from a function with one target attribute (or missing) to one with a > different target attribute calls target_reinit, which is unfortunately quite > inefficient, doesn't cache anything and, what's worse, initializes rtl > generation and cleans it up afterwards for init_expmed, init_expr_target, > init_set_costs and ira_init purposes. When set_cfun with such target_reinit > side-effect is called after prepare_function_start call (called very early > in tree_rest_of_compilation), it clears *crtl and thus e.g. during expansion > when allocating pseudos they don't start at LAST_VIRTUAL_REGISTER + 1, but > at 0. > > For 4.7 it would be nice if we could cache results of as many of these init_* > functions as possible, at least for those that are used before rtl > generation (e.g. init_set_costs), and for init_* calls whose results are > only ever used in between expand_gimple_cfg and following > free_after_compilation we could just call them at the start of > expand_gimple_cfg if target attribute changed since last expansion (e.g. > ira_init would be hard to save/restore). > > For 4.6 that would be too risky, so this patch instead just saves/restores > what free_after_compilation clobbers. Initially I thought I'd need to > register those with GC, but looking at the source I think ggc_collect () > can't be called during target_reinit (it would break quite a few passes that > do set_cfun at various spots temporarily if set_cfun could collect). > > The cgraph verification changes are just to make the verification cheaper > (target_reinit is very costly and if we can avoid it easily...). > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? The cgraph bits are ok. Thanks, Richard. > 2011-02-02 Jakub Jelinek <jakub@redhat.com> > > PR target/47564 > * toplev.c (target_reinit): Save and restore *crtl and regno_reg_rtx > around backend_init_target and lang_dependent_init_target calls. > * cgraphunit.c (cgraph_debug_gimple_stmt): New function. > (verify_cgraph_node): Don't call set_cfun here. Use > cgraph_debug_gimple_stmt instead of debug_gimple_stmt. > Set error_found for incorrectly represented calls to thunks. > > * gcc.target/i386/pr47564.c: New test. > > --- gcc/toplev.c.jj 2011-02-01 16:23:54.000000000 +0100 > +++ gcc/toplev.c 2011-02-02 10:40:15.000000000 +0100 > @@ -1780,11 +1780,33 @@ lang_dependent_init (const char *name) > void > target_reinit (void) > { > + struct rtl_data saved_x_rtl; > + rtx *saved_regno_reg_rtx; > + > + /* Save *crtl and regno_reg_rtx around the reinitialization > + to allow target_reinit being called even after prepare_function_start. */ > + saved_regno_reg_rtx = regno_reg_rtx; > + if (saved_regno_reg_rtx) > + { > + saved_x_rtl = *crtl; > + memset (crtl, '\0', sizeof (*crtl)); > + regno_reg_rtx = NULL; > + } > + > /* Reinitialize RTL backend. */ > backend_init_target (); > > /* Reinitialize lang-dependent parts. */ > lang_dependent_init_target (); > + > + /* And restore it at the end, as free_after_compilation from > + expand_dummy_function_end clears it. */ > + if (saved_regno_reg_rtx) > + { > + *crtl = saved_x_rtl; > + regno_reg_rtx = saved_regno_reg_rtx; > + saved_regno_reg_rtx = NULL; > + } > } > > void > --- gcc/cgraphunit.c.jj 2011-01-31 14:11:30.000000000 +0100 > +++ gcc/cgraphunit.c 2011-02-01 15:10:50.000000000 +0100 > @@ -440,13 +440,22 @@ verify_edge_count_and_frequency (struct > return error_found; > } > > +/* Switch to THIS_CFUN if needed and print STMT to stderr. */ > +static void > +cgraph_debug_gimple_stmt (struct function *this_cfun, gimple stmt) > +{ > + /* debug_gimple_stmt needs correct cfun */ > + if (cfun != this_cfun) > + set_cfun (this_cfun); > + debug_gimple_stmt (stmt); > +} > + > /* Verify cgraph nodes of given cgraph node. */ > DEBUG_FUNCTION void > verify_cgraph_node (struct cgraph_node *node) > { > struct cgraph_edge *e; > struct function *this_cfun = DECL_STRUCT_FUNCTION (node->decl); > - struct function *saved_cfun = cfun; > basic_block this_block; > gimple_stmt_iterator gsi; > bool error_found = false; > @@ -455,8 +464,6 @@ verify_cgraph_node (struct cgraph_node * > return; > > timevar_push (TV_CGRAPH_VERIFY); > - /* debug_generic_stmt needs correct cfun */ > - set_cfun (this_cfun); > for (e = node->callees; e; e = e->next_callee) > if (e->aux) > { > @@ -499,7 +506,7 @@ verify_cgraph_node (struct cgraph_node * > error ("An indirect edge from %s is not marked as indirect or has " > "associated indirect_info, the corresponding statement is: ", > identifier_to_locale (cgraph_node_name (e->caller))); > - debug_gimple_stmt (e->call_stmt); > + cgraph_debug_gimple_stmt (this_cfun, e->call_stmt); > error_found = true; > } > } > @@ -642,7 +649,7 @@ verify_cgraph_node (struct cgraph_node * > if (e->aux) > { > error ("shared call_stmt:"); > - debug_gimple_stmt (stmt); > + cgraph_debug_gimple_stmt (this_cfun, stmt); > error_found = true; > } > if (!e->indirect_unknown_callee) > @@ -676,7 +683,8 @@ verify_cgraph_node (struct cgraph_node * > { > error ("a call to thunk improperly represented " > "in the call graph:"); > - debug_gimple_stmt (stmt); > + cgraph_debug_gimple_stmt (this_cfun, stmt); > + error_found = true; > } > } > else if (decl) > @@ -685,14 +693,14 @@ verify_cgraph_node (struct cgraph_node * > "corresponding to a call_stmt with " > "a known declaration:"); > error_found = true; > - debug_gimple_stmt (e->call_stmt); > + cgraph_debug_gimple_stmt (this_cfun, e->call_stmt); > } > e->aux = (void *)1; > } > else if (decl) > { > error ("missing callgraph edge for call stmt:"); > - debug_gimple_stmt (stmt); > + cgraph_debug_gimple_stmt (this_cfun, stmt); > error_found = true; > } > } > @@ -710,7 +718,7 @@ verify_cgraph_node (struct cgraph_node * > error ("edge %s->%s has no corresponding call_stmt", > identifier_to_locale (cgraph_node_name (e->caller)), > identifier_to_locale (cgraph_node_name (e->callee))); > - debug_gimple_stmt (e->call_stmt); > + cgraph_debug_gimple_stmt (this_cfun, e->call_stmt); > error_found = true; > } > e->aux = 0; > @@ -721,7 +729,7 @@ verify_cgraph_node (struct cgraph_node * > { > error ("an indirect edge from %s has no corresponding call_stmt", > identifier_to_locale (cgraph_node_name (e->caller))); > - debug_gimple_stmt (e->call_stmt); > + cgraph_debug_gimple_stmt (this_cfun, e->call_stmt); > error_found = true; > } > e->aux = 0; > @@ -732,7 +740,6 @@ verify_cgraph_node (struct cgraph_node * > dump_cgraph_node (stderr, node); > internal_error ("verify_cgraph_node failed"); > } > - set_cfun (saved_cfun); > timevar_pop (TV_CGRAPH_VERIFY); > } > > --- gcc/testsuite/gcc.target/i386/pr47564.c.jj 2011-02-02 10:24:34.000000000 +0100 > +++ gcc/testsuite/gcc.target/i386/pr47564.c 2011-02-02 10:27:22.000000000 +0100 > @@ -0,0 +1,42 @@ > +/* PR target/47564 */ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -msse2" } */ > + > +static inline unsigned long long > +foo (const unsigned char *p) > +{ > + return 1; > +} > + > +__attribute__ ((__target__ ("sse4"))) void > +bar (unsigned long long *x, const void *b, long long m) > +{ > + const unsigned char *p = (const unsigned char *) b; > + const unsigned char *e = p + m; > + unsigned int l = *x; > + unsigned long long n = l; > + > + if ((e - p) >= 8192) > + { > + while ((e - p) >= 128) > + { > + n = __builtin_ia32_crc32di (n, foo (p)); > + n = __builtin_ia32_crc32di (n, foo (p)); > + n = __builtin_ia32_crc32di (n, foo (p)); > + n = __builtin_ia32_crc32di (n, foo (p)); > + n = __builtin_ia32_crc32di (n, foo (p)); > + n = __builtin_ia32_crc32di (n, foo (p)); > + n = __builtin_ia32_crc32di (n, foo (p)); > + n = __builtin_ia32_crc32di (n, foo (p)); > + n = __builtin_ia32_crc32di (n, foo (p)); > + } > + } > + > + while ((e - p) >= 16) > + { > + n = __builtin_ia32_crc32di (n, foo (p)); > + n = __builtin_ia32_crc32di (n, foo (p)); > + } > + l = n; > + *x = l; > +} > > Jakub >
On 02/02/2011 06:07 AM, Jakub Jelinek wrote: > PR target/47564 > * toplev.c (target_reinit): Save and restore *crtl and regno_reg_rtx > around backend_init_target and lang_dependent_init_target calls. > * cgraphunit.c (cgraph_debug_gimple_stmt): New function. > (verify_cgraph_node): Don't call set_cfun here. Use > cgraph_debug_gimple_stmt instead of debug_gimple_stmt. > Set error_found for incorrectly represented calls to thunks. > > * gcc.target/i386/pr47564.c: New test. Ok. r~
different target attribute calls target_reinit, which is unfortunately quite inefficient, doesn't cache anything and, what's worse, initializes rtl generation and cleans it up afterwards for init_expmed, init_expr_target, init_set_costs and ira_init purposes. When set_cfun with such target_reinit side-effect is called after prepare_function_start call (called very early in tree_rest_of_compilation), it clears *crtl and thus e.g. during expansion when allocating pseudos they don't start at LAST_VIRTUAL_REGISTER + 1, but at 0. For 4.7 it would be nice if we could cache results of as many of these init_* functions as possible, at least for those that are used before rtl generation (e.g. init_set_costs), and for init_* calls whose results are only ever used in between expand_gimple_cfg and following free_after_compilation we could just call them at the start of expand_gimple_cfg if target attribute changed since last expansion (e.g. ira_init would be hard to save/restore). For 4.6 that would be too risky, so this patch instead just saves/restores what free_after_compilation clobbers. Initially I thought I'd need to register those with GC, but looking at the source I think ggc_collect () can't be called during target_reinit (it would break quite a few passes that do set_cfun at various spots temporarily if set_cfun could collect). The cgraph verification changes are just to make the verification cheaper (target_reinit is very costly and if we can avoid it easily...). Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2011-02-02 Jakub Jelinek <jakub@redhat.com> PR target/47564 * toplev.c (target_reinit): Save and restore *crtl and regno_reg_rtx around backend_init_target and lang_dependent_init_target calls. * cgraphunit.c (cgraph_debug_gimple_stmt): New function. (verify_cgraph_node): Don't call set_cfun here. Use cgraph_debug_gimple_stmt instead of debug_gimple_stmt. Set error_found for incorrectly represented calls to thunks. * gcc.target/i386/pr47564.c: New test. --- gcc/toplev.c.jj 2011-02-01 16:23:54.000000000 +0100 +++ gcc/toplev.c 2011-02-02 10:40:15.000000000 +0100 @@ -1780,11 +1780,33 @@ lang_dependent_init (const char *name) void target_reinit (void) { + struct rtl_data saved_x_rtl; + rtx *saved_regno_reg_rtx; + + /* Save *crtl and regno_reg_rtx around the reinitialization + to allow target_reinit being called even after prepare_function_start. */ + saved_regno_reg_rtx = regno_reg_rtx; + if (saved_regno_reg_rtx) + { + saved_x_rtl = *crtl; + memset (crtl, '\0', sizeof (*crtl)); + regno_reg_rtx = NULL; + } + /* Reinitialize RTL backend. */ backend_init_target (); /* Reinitialize lang-dependent parts. */ lang_dependent_init_target (); + + /* And restore it at the end, as free_after_compilation from + expand_dummy_function_end clears it. */ + if (saved_regno_reg_rtx) + { + *crtl = saved_x_rtl; + regno_reg_rtx = saved_regno_reg_rtx; + saved_regno_reg_rtx = NULL; + } } void --- gcc/cgraphunit.c.jj 2011-01-31 14:11:30.000000000 +0100 +++ gcc/cgraphunit.c 2011-02-01 15:10:50.000000000 +0100 @@ -440,13 +440,22 @@ verify_edge_count_and_frequency (struct return error_found; } +/* Switch to THIS_CFUN if needed and print STMT to stderr. */ +static void +cgraph_debug_gimple_stmt (struct function *this_cfun, gimple stmt) +{ + /* debug_gimple_stmt needs correct cfun */ + if (cfun != this_cfun) + set_cfun (this_cfun); + debug_gimple_stmt (stmt); +} + /* Verify cgraph nodes of given cgraph node. */ DEBUG_FUNCTION void verify_cgraph_node (struct cgraph_node *node) { struct cgraph_edge *e; struct function *this_cfun = DECL_STRUCT_FUNCTION (node->decl); - struct function *saved_cfun = cfun; basic_block this_block; gimple_stmt_iterator gsi; bool error_found = false; @@ -455,8 +464,6 @@ verify_cgraph_node (struct cgraph_node * return; timevar_push (TV_CGRAPH_VERIFY); - /* debug_generic_stmt needs correct cfun */ - set_cfun (this_cfun); for (e = node->callees; e; e = e->next_callee) if (e->aux) { @@ -499,7 +506,7 @@ verify_cgraph_node (struct cgraph_node * error ("An indirect edge from %s is not marked as indirect or has " "associated indirect_info, the corresponding statement is: ", identifier_to_locale (cgraph_node_name (e->caller))); - debug_gimple_stmt (e->call_stmt); + cgraph_debug_gimple_stmt (this_cfun, e->call_stmt); error_found = true; } } @@ -642,7 +649,7 @@ verify_cgraph_node (struct cgraph_node * if (e->aux) { error ("shared call_stmt:"); - debug_gimple_stmt (stmt); + cgraph_debug_gimple_stmt (this_cfun, stmt); error_found = true; } if (!e->indirect_unknown_callee) @@ -676,7 +683,8 @@ verify_cgraph_node (struct cgraph_node * { error ("a call to thunk improperly represented " "in the call graph:"); - debug_gimple_stmt (stmt); + cgraph_debug_gimple_stmt (this_cfun, stmt); + error_found = true; } } else if (decl) @@ -685,14 +693,14 @@ verify_cgraph_node (struct cgraph_node * "corresponding to a call_stmt with " "a known declaration:"); error_found = true; - debug_gimple_stmt (e->call_stmt); + cgraph_debug_gimple_stmt (this_cfun, e->call_stmt); } e->aux = (void *)1; } else if (decl) { error ("missing callgraph edge for call stmt:"); - debug_gimple_stmt (stmt); + cgraph_debug_gimple_stmt (this_cfun, stmt); error_found = true; } } @@ -710,7 +718,7 @@ verify_cgraph_node (struct cgraph_node * error ("edge %s->%s has no corresponding call_stmt", identifier_to_locale (cgraph_node_name (e->caller)), identifier_to_locale (cgraph_node_name (e->callee))); - debug_gimple_stmt (e->call_stmt); + cgraph_debug_gimple_stmt (this_cfun, e->call_stmt); error_found = true; } e->aux = 0; @@ -721,7 +729,7 @@ verify_cgraph_node (struct cgraph_node * { error ("an indirect edge from %s has no corresponding call_stmt", identifier_to_locale (cgraph_node_name (e->caller))); - debug_gimple_stmt (e->call_stmt); + cgraph_debug_gimple_stmt (this_cfun, e->call_stmt); error_found = true; } e->aux = 0; @@ -732,7 +740,6 @@ verify_cgraph_node (struct cgraph_node * dump_cgraph_node (stderr, node); internal_error ("verify_cgraph_node failed"); } - set_cfun (saved_cfun); timevar_pop (TV_CGRAPH_VERIFY); } --- gcc/testsuite/gcc.target/i386/pr47564.c.jj 2011-02-02 10:24:34.000000000 +0100 +++ gcc/testsuite/gcc.target/i386/pr47564.c 2011-02-02 10:27:22.000000000 +0100 @@ -0,0 +1,42 @@ +/* PR target/47564 */ +/* { dg-do compile } */ +/* { dg-options "-O2 -msse2" } */ + +static inline unsigned long long +foo (const unsigned char *p) +{ + return 1; +} + +__attribute__ ((__target__ ("sse4"))) void +bar (unsigned long long *x, const void *b, long long m) +{ + const unsigned char *p = (const unsigned char *) b; + const unsigned char *e = p + m; + unsigned int l = *x; + unsigned long long n = l; + + if ((e - p) >= 8192) + { + while ((e - p) >= 128) + { + n = __builtin_ia32_crc32di (n, foo (p)); + n = __builtin_ia32_crc32di (n, foo (p)); + n = __builtin_ia32_crc32di (n, foo (p)); + n = __builtin_ia32_crc32di (n, foo (p)); + n = __builtin_ia32_crc32di (n, foo (p)); + n = __builtin_ia32_crc32di (n, foo (p)); + n = __builtin_ia32_crc32di (n, foo (p)); + n = __builtin_ia32_crc32di (n, foo (p)); + n = __builtin_ia32_crc32di (n, foo (p)); + } + } + + while ((e - p) >= 16) + { + n = __builtin_ia32_crc32di (n, foo (p)); + n = __builtin_ia32_crc32di (n, foo (p)); + } + l = n; + *x = l; +}