Re: [GOOGLE] Fix incorrect detection of recursive calls during LIPO IPA inlining

[Xinliang David Li <davidxl at google dot com>]

Looks good to me.

David

On Thu, Apr 10, 2014 at 2:04 PM, Teresa Johnson <tejohnson@google.com> wrote:
> This patch fixes an issue where self-recursive calls were missed
> during ipa inlining in LIPO compiles, since the resolved nodes were
> not checked. When a self-recursive call was inlined incorrectly,
> ipa_inline redirected the edge to the resolved node, leading to
> a cycle in the cgraph that wasn't expected and resulted in an
> infinite loop in estimate_calls_size_and_time.
>
> Also fix an inconsistency in the declaration of include_all_aux
> that was exposed when l-ipo.h was included in cgraph.
>
> Google ref b/13912450.
>
> Bootstrapped and tested. Ok for google/4_8?
>
> Thanks,
> Teresa
>
> 2014-04-10  Teresa Johnson  <tejohnson@google.com>
>
>         * cgraph.h (cgraph_edge_recursive_p): Check the resolved
>         node in LIPO compiles.
>         * l-ipo.h (include_all_aux): Fix declaration that didn't
>         match definition.
>         * Makefile.in (CGRAPH_H): Include l-ipo.h.
>
> Index: cgraph.h
> ===================================================================
> --- cgraph.h    (revision 209280)
> +++ cgraph.h    (working copy)
> @@ -28,6 +28,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "basic-block.h"
>  #include "function.h"
>  #include "ipa-ref.h"
> +#include "l-ipo.h"
>
>  /* Symbol table consists of functions and variables.
>     TODO: add labels, constant pool and aliases.  */
> @@ -1388,10 +1389,15 @@ static inline bool
>  cgraph_edge_recursive_p (struct cgraph_edge *e)
>  {
>    struct cgraph_node *callee = cgraph_function_or_thunk_node (e->callee, NULL);
> +  struct cgraph_node *caller = e->caller;
>    if (e->caller->global.inlined_to)
> -    return e->caller->global.inlined_to->symbol.decl == callee->symbol.decl;
> -  else
> -    return e->caller->symbol.decl == callee->symbol.decl;
> +    caller = e->caller->global.inlined_to;
> +  if (L_IPO_COMP_MODE && cgraph_pre_profiling_inlining_done)
> +    {
> +      callee = cgraph_lipo_get_resolved_node (callee->symbol.decl);
> +      caller = cgraph_lipo_get_resolved_node (caller->symbol.decl);
> +    }
> +  return (caller->symbol.decl == callee->symbol.decl);
>  }
>
>  /* Return true if the TM_CLONE bit is set for a given FNDECL.  */
> Index: l-ipo.h
> ===================================================================
> --- l-ipo.h     (revision 209280)
> +++ l-ipo.h     (working copy)
> @@ -44,7 +44,7 @@ extern unsigned primary_module_id;
>
>  /* Current module id.  */
>  extern unsigned current_module_id;
> -extern unsigned include_all_aux;
> +extern bool include_all_aux;
>  extern struct gcov_module_info **module_infos;
>  extern int is_last_module (unsigned mod_id);
>
> Index: Makefile.in
> ===================================================================
> --- Makefile.in (revision 209280)
> +++ Makefile.in (working copy)
> @@ -904,7 +904,8 @@ CFGLOOP_H = cfgloop.h $(BASIC_BLOCK_H) double-int.
>  IPA_UTILS_H = ipa-utils.h $(TREE_H) $(CGRAPH_H)
>  IPA_REFERENCE_H = ipa-reference.h $(BITMAP_H) $(TREE_H)
>  CGRAPH_H = cgraph.h $(VEC_H) $(TREE_H) $(BASIC_BLOCK_H) $(FUNCTION_H) \
> -       cif-code.def ipa-ref.h ipa-ref-inline.h $(LINKER_PLUGIN_API_H) is-a.h
> +       cif-code.def ipa-ref.h ipa-ref-inline.h $(LINKER_PLUGIN_API_H) is-a.h \
> +       l-ipo.h
>  DF_H = df.h $(BITMAP_H) $(REGSET_H) sbitmap.h $(BASIC_BLOCK_H) \
>         alloc-pool.h $(TIMEVAR_H)
>  VALTRACK_H = valtrack.h $(BITMAP_H) $(DF_H) $(RTL_H) $(BASIC_BLOCK_H) \
>
>
> --
> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413