Please review the latest patch. SPEC2k FDO testing pass.
Thanks,
David
On Wed, Apr 20, 2011 at 11:22 AM, Xinliang David Li <davi...@google.com> wrote:
> Here is the revised patch. Basic FDO testing went fine. I still saw
> the ipa-inline assertion in SPEC FDO. Will run it when the regression
> is fixed.
>
> Thanks,
>
> David
>
> On Tue, Apr 19, 2011 at 5:33 PM, Jan Hubicka <hubi...@ucw.cz> wrote:
>>> On Tue, Apr 19, 2011 at 4:49 PM, Jan Hubicka <hubi...@ucw.cz> wrote:
>>> >> Actually, among all the choices, funcdef_no is probably the most dense
>>> >> one -- it is for function decl with definition only. In LIPO, the
>>> >
>>> > Yes, funddef_no is densiest, but we don't really need great density here
>>> > (in many other places we index arrays by cgraph_uid - it is intended for
>>> > that purpose and we pay some attention to recycle unused nodes).
>>> >
>>>
>>> That does not mean it is right to use sparse ids:) DECL_UID will be
>>> the worst amongst them.
>>
>> Sure, that is why I suggested cgraph->uid. That one is kept dense and it
>> also
>> tracks cgraph node creation order. Unlike pid it counts also functions w/o
>> bodies.
>>> > We only care to avoid divergence in the indexes in between instrumentation
>>> > and feedback compilation. With the IPA pass organizatoin the compiler
>>> > doesn't
>>> > really diverge until the profile pass, so it seems to me that all should
>>> > be safe.
>>>
>>> When I said 'fragile' -- I meant it depends on the optimization pass
>>> phase ordering which can change in the future.
>>
>> Well, that is the case of all of them (passes can create function bodies that
>> can make funcdef_no also diverge). This is the case of couple passes
>> already,
>> especially OMP lowering and friends.
>>
>>> Ok, I will throw away pid and use funcdef_no for now. For future
>>> replacement for the function ids, please consider the following
>>> desired properties:
>>>
>>> 1) The id sequence does not depend on optimization passes -- only
>>> depend on source/parsing order;
>>
>> It depends on optimization, too. This is why we actually have cgraph->order
>> that is used for -fno-toplevel-reorder and is similar to funcdef_no, but
>> assigned at finalization time.
>>
>>> 2) It is dense and sequential for defined functions
>>> a) it has proven to be very useful to use nice looking, sequential
>>> ids in triaging coverage mismatch related problems (the tree dump
>>> should also show the function id);
>>
>> You get the cgraph uids in the dumps already.
>>> b) it can be very useful in bug triaging via binary search by
>>> specifying ranges of function ids (to enable optimizations etc).
>>
>> But as you wish, we can process with fundef_no first and then discuss
>> removal of that field later.
>>
>> Honza
>>>
>>> Thanks,
>>>
>>> David
>>>
>>>
>>> >
>>> > Honza
>>> >>
>>> >> David
>>> >>
>>> >>
>>> >>
>>> >> >
>>> >> > Honza
>>> >> >
>>> >
>>
>
Index: cgraph.c
===================================================================
--- cgraph.c (revision 172781)
+++ cgraph.c (working copy)
@@ -142,9 +142,6 @@ int cgraph_max_uid;
/* Maximal uid used in cgraph edges. */
int cgraph_edge_max_uid;
-/* Maximal pid used for profiling */
-int cgraph_max_pid;
-
/* Set when whole unit has been analyzed so we can access global info. */
bool cgraph_global_info_ready = false;
@@ -472,7 +469,6 @@ cgraph_create_node_1 (void)
struct cgraph_node *node = cgraph_allocate_node ();
node->next = cgraph_nodes;
- node->pid = -1;
node->order = cgraph_order++;
if (cgraph_nodes)
cgraph_nodes->previous = node;
@@ -1827,8 +1823,7 @@ dump_cgraph_node (FILE *f, struct cgraph
struct cgraph_edge *edge;
int indirect_calls_count = 0;
- fprintf (f, "%s/%i(%i)", cgraph_node_name (node), node->uid,
- node->pid);
+ fprintf (f, "%s/%i", cgraph_node_name (node), node->uid);
dump_addr (f, " @", (void *)node);
if (DECL_ASSEMBLER_NAME_SET_P (node->decl))
fprintf (f, " (asm: %s)", IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (node->decl)));
Index: cgraph.h
===================================================================
--- cgraph.h (revision 172781)
+++ cgraph.h (working copy)
@@ -200,9 +200,6 @@ struct GTY((chain_next ("%h.next"), chai
/* Ordering of all cgraph nodes. */
int order;
- /* unique id for profiling. pid is not suitable because of different
- number of cfg nodes with -fprofile-generate and -fprofile-use */
- int pid;
enum ld_plugin_symbol_resolution resolution;
/* Set when function must be output for some reason. The primary
@@ -472,7 +469,6 @@ extern GTY(()) struct cgraph_node *cgrap
extern GTY(()) int cgraph_n_nodes;
extern GTY(()) int cgraph_max_uid;
extern GTY(()) int cgraph_edge_max_uid;
-extern GTY(()) int cgraph_max_pid;
extern bool cgraph_global_info_ready;
enum cgraph_state
{
@@ -730,6 +726,8 @@ void cgraph_clone_inlined_nodes (struct
void compute_inline_parameters (struct cgraph_node *);
cgraph_inline_failed_t cgraph_edge_inlinable_p (struct cgraph_edge *);
+void cgraph_init_node_map (void);
+void cgraph_del_node_map (void);
/* Create a new static variable of type TYPE. */
tree add_new_static_var (tree type);
Index: value-prof.c
===================================================================
--- value-prof.c (revision 172781)
+++ value-prof.c (working copy)
@@ -1059,35 +1059,64 @@ gimple_mod_subtract_transform (gimple_st
return true;
}
-static struct cgraph_node** pid_map = NULL;
+typedef struct
+{
+ struct cgraph_node *n;
+} cgraph_node_ptr_t;
-/* Initialize map of pids (pid -> cgraph node) */
+DEF_VEC_O (cgraph_node_ptr_t);
+DEF_VEC_ALLOC_O (cgraph_node_ptr_t, heap);
-static void
-init_pid_map (void)
+static VEC(cgraph_node_ptr_t, heap) *cgraph_node_map = NULL;
+
+/* Initialize map from FUNCDEF_NO to CGRAPH_NODE. */
+
+void
+cgraph_init_node_map (void)
{
struct cgraph_node *n;
- if (pid_map != NULL)
- return;
-
- pid_map = XCNEWVEC (struct cgraph_node*, cgraph_max_pid);
+ if (get_last_funcdef_no ())
+ VEC_safe_grow_cleared (cgraph_node_ptr_t, heap,
+ cgraph_node_map, get_last_funcdef_no ());
for (n = cgraph_nodes; n; n = n->next)
{
- if (n->pid != -1)
- pid_map [n->pid] = n;
+ if (DECL_STRUCT_FUNCTION (n->decl))
+ VEC_index (cgraph_node_ptr_t, cgraph_node_map,
+ DECL_STRUCT_FUNCTION (n->decl)->funcdef_no)->n = n;
}
}
+/* Delete the CGRAPH_NODE_MAP. */
+
+void
+cgraph_del_node_map (void)
+{
+ VEC_free (cgraph_node_ptr_t, heap, cgraph_node_map);
+ cgraph_node_map = NULL;
+}
+
/* Return cgraph node for function with pid */
static inline struct cgraph_node*
-find_func_by_pid (int pid)
+find_func_by_funcdef_no (int func_id)
{
- init_pid_map ();
+ int max_id = get_last_funcdef_no ();
+ if (func_id >= max_id || VEC_index (cgraph_node_ptr_t,
+ cgraph_node_map,
+ func_id)->n == NULL)
+ {
+ if (flag_profile_correction)
+ inform (DECL_SOURCE_LOCATION (current_function_decl),
+ "Inconsistent profile: indirect call target (%d) does not exist", func_id);
+ else
+ error ("Inconsistent profile: indirect call target (%d) does not exist", func_id);
+
+ return NULL;
+ }
- return pid_map [pid];
+ return VEC_index (cgraph_node_ptr_t, cgraph_node_map, func_id)->n;
}
/* Perform sanity check on the indirect call target. Due to race conditions,
@@ -1285,7 +1314,7 @@ gimple_ic_transform (gimple stmt)
prob = (count * REG_BR_PROB_BASE + all / 2) / all;
else
prob = 0;
- direct_call = find_func_by_pid ((int)val);
+ direct_call = find_func_by_funcdef_no ((int)val);
if (direct_call == NULL)
return false;
Index: cgraphunit.c
===================================================================
--- cgraphunit.c (revision 172781)
+++ cgraphunit.c (working copy)
@@ -348,7 +348,6 @@ cgraph_finalize_function (tree decl, boo
if (node->local.finalized)
cgraph_reset_node (node);
- node->pid = cgraph_max_pid ++;
notice_global_symbol (decl);
node->local.finalized = true;
node->lowered = DECL_STRUCT_FUNCTION (decl)->cfg != NULL;
Index: function.c
===================================================================
--- function.c (revision 172781)
+++ function.c (working copy)
@@ -4379,6 +4379,13 @@ get_next_funcdef_no (void)
return funcdef_no++;
}
+/* Return value of funcdef. */
+int
+get_last_funcdef_no (void)
+{
+ return funcdef_no;
+}
+
/* Allocate a function structure for FNDECL and set its contents
to the defaults. Set cfun to the newly-allocated object.
Some of the helper functions invoked during initialization assume
Index: function.h
===================================================================
--- function.h (revision 172781)
+++ function.h (working copy)
@@ -755,6 +755,7 @@ extern bool reference_callee_copied (CUM
extern void used_types_insert (tree);
extern int get_next_funcdef_no (void);
+extern int get_last_funcdef_no (void);
/* In predict.c */
extern bool optimize_function_for_size_p (struct function *);
Index: tree-profile.c
===================================================================
--- tree-profile.c (revision 172781)
+++ tree-profile.c (working copy)
@@ -369,7 +369,7 @@ gimple_gen_ic_func_profiler (void)
ptr_var = force_gimple_operand_gsi (&gsi, ic_void_ptr_var,
true, NULL_TREE, true,
GSI_SAME_STMT);
- tree_uid = build_int_cst (gcov_type_node, c_node->pid);
+ tree_uid = build_int_cst (gcov_type_node, current_function_funcdef_no);
stmt1 = gimple_build_call (tree_indirect_call_profiler_fn, 4,
counter_ptr, tree_uid, cur_func, ptr_var);
gsi_insert_before (&gsi, stmt1, GSI_SAME_STMT);
@@ -454,6 +454,8 @@ tree_profiling (void)
if (cgraph_state == CGRAPH_STATE_FINISHED)
return 0;
+ cgraph_init_node_map();
+
for (node = cgraph_nodes; node; node = node->next)
{
if (!node->analyzed
@@ -548,6 +550,7 @@ tree_profiling (void)
pop_cfun ();
}
+ cgraph_del_node_map();
return 0;
}
