Fix target_reinit (PR target/47564)

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

	Jakub

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~