FDO usability: pid handling

FDO usability: pid handling

[Xinliang David Li]

[-- Attachment #1: Type: text/plain, Size: 616 bytes --]

Hi, Insane value profile data may contain indirect call targets with
wrong (corrupted) pids.  r172276 solves the problem when the pid
refers to a bogus target that is still 'alive'. This patch addresses
the issue when the bogus target is already eliminated or pid is too
large.

OK after testing? (SPEC FDO testing is currently blocked by some regressions).

Thanks,

David



2011-04-19  Xinliang David Li  <davidxl@google.com>
	
	* cgraph.h: New function declaration.
	* cgraph.c (cgraph_remove_node): Remove from pid map.
	* value-prof.c (find_func_by_pid): Handle insane pid.
	(cgraph_remove_pid): New function.

[-- Attachment #2: fdo_pid.p --]
[-- Type: text/x-pascal, Size: 2077 bytes --]

Index: cgraph.h
===================================================================
--- cgraph.h	(revision 172721)
+++ cgraph.h	(working copy)
@@ -732,6 +732,8 @@ void compute_inline_parameters (struct c
 cgraph_inline_failed_t cgraph_edge_inlinable_p (struct cgraph_edge *);
 
 
+void cgraph_remove_pid (struct cgraph_node *);
+
 /* Create a new static variable of type TYPE.  */
 tree add_new_static_var (tree type);
 
Index: cgraph.c
===================================================================
--- cgraph.c	(revision 172721)
+++ cgraph.c	(working copy)
@@ -1478,6 +1478,7 @@ cgraph_remove_node (struct cgraph_node *
   cgraph_call_node_removal_hooks (node);
   cgraph_node_remove_callers (node);
   cgraph_node_remove_callees (node);
+  cgraph_remove_pid (node);
   ipa_remove_all_references (&node->ref_list);
   ipa_remove_all_refering (&node->ref_list);
   VEC_free (ipa_opt_pass, heap,
Index: value-prof.c
===================================================================
--- value-prof.c	(revision 172721)
+++ value-prof.c	(working copy)
@@ -1083,13 +1083,35 @@ init_pid_map (void)
 /* Return cgraph node for function with pid */
 
 static inline struct cgraph_node*
-find_func_by_pid (int	pid)
+find_func_by_pid (int pid)
 {
   init_pid_map ();
 
+  if (pid >= cgraph_max_pid)
+    {
+      if (flag_profile_correction)
+        inform (DECL_SOURCE_LOCATION (current_function_decl),
+                "Inconsistent profile: indirect call target (%d) does not exist", pid);
+      else
+        error ("Inconsistent profile: indirect call target (%d) does not exist", pid);
+
+      return NULL;
+    }
+
   return pid_map [pid];
 }
 
+/* Remove a cgraph node N from maps of pids  */
+
+void
+cgraph_remove_pid (struct cgraph_node *n)
+{
+  if (pid_map == NULL || n->pid < 0)
+    return;
+
+  pid_map [n->pid] = NULL;
+}
+
 /* Perform sanity check on the indirect call target. Due to race conditions,
    false function target may be attributed to an indirect call site. If the
    call expression type mismatches with the target function's type, expand_call

Re: FDO usability: pid handling

[Jan Hubicka]

> Hi, Insane value profile data may contain indirect call targets with
> wrong (corrupted) pids.  r172276 solves the problem when the pid
> refers to a bogus target that is still 'alive'. This patch addresses
> the issue when the bogus target is already eliminated or pid is too
> large.
> 
> OK after testing? (SPEC FDO testing is currently blocked by some regressions).

I will look into the FDO inliner ICEs.  I added sanity check for profile updates
and it seems that insane profiles are mishandled somewhere.

> Index: cgraph.c
> ===================================================================
> --- cgraph.c	(revision 172721)
> +++ cgraph.c	(working copy)
> @@ -1478,6 +1478,7 @@ cgraph_remove_node (struct cgraph_node *
>    cgraph_call_node_removal_hooks (node);
>    cgraph_node_remove_callers (node);
>    cgraph_node_remove_callees (node);
> +  cgraph_remove_pid (node);
>    ipa_remove_all_references (&node->ref_list);
>    ipa_remove_all_refering (&node->ref_list);
>    VEC_free (ipa_opt_pass, heap,

profiling is now run as IPA pass. This means that we compute pid map and
nothing can remove nodes before we read in the profile.  So it this is
unnecesary to hook PID array updating into cgraph_remove_node.

> Index: value-prof.c
> ===================================================================
> --- value-prof.c	(revision 172721)
> +++ value-prof.c	(working copy)
> @@ -1083,13 +1083,35 @@ init_pid_map (void)
>  /* Return cgraph node for function with pid */
>  
>  static inline struct cgraph_node*
> -find_func_by_pid (int	pid)
> +find_func_by_pid (int pid)
>  {
>    init_pid_map ();
>  
> +  if (pid >= cgraph_max_pid)
> +    {
> +      if (flag_profile_correction)
> +        inform (DECL_SOURCE_LOCATION (current_function_decl),
> +                "Inconsistent profile: indirect call target (%d) does not exist", pid);
> +      else
> +        error ("Inconsistent profile: indirect call target (%d) does not exist", pid);
> +
> +      return NULL;
> +    }
> +
>    return pid_map [pid];

PIDs are not dense, because functions might've been removed from before we get
to ipa-profile.  I would suggest making pid_map VECtor to be more consistent
with rest of GCC sourc, making init_pid_map to allocate it cleared and add also
test that VEC_index (pid_map, pid) is not NULL in addition to the bound check
above.

Also we ought to free pids once the IPA profiling pass is done. I.e. at the end of tree_profling
function.

Honza

Re: FDO usability: pid handling

[Xinliang David Li]

On Tue, Apr 19, 2011 at 2:38 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Hi, Insane value profile data may contain indirect call targets with
>> wrong (corrupted) pids.  r172276 solves the problem when the pid
>> refers to a bogus target that is still 'alive'. This patch addresses
>> the issue when the bogus target is already eliminated or pid is too
>> large.
>>
>> OK after testing? (SPEC FDO testing is currently blocked by some regressions).
>
> I will look into the FDO inliner ICEs.  I added sanity check for profile updates
> and it seems that insane profiles are mishandled somewhere.
>
>> Index: cgraph.c
>> ===================================================================
>> --- cgraph.c  (revision 172721)
>> +++ cgraph.c  (working copy)
>> @@ -1478,6 +1478,7 @@ cgraph_remove_node (struct cgraph_node *
>>    cgraph_call_node_removal_hooks (node);
>>    cgraph_node_remove_callers (node);
>>    cgraph_node_remove_callees (node);
>> +  cgraph_remove_pid (node);
>>    ipa_remove_all_references (&node->ref_list);
>>    ipa_remove_all_refering (&node->ref_list);
>>    VEC_free (ipa_opt_pass, heap,
>
> profiling is now run as IPA pass. This means that we compute pid map and
> nothing can remove nodes before we read in the profile.  So it this is
> unnecesary to hook PID array updating into cgraph_remove_node.

Right -- with the current phase ordering it should be safe -- the pid
map is created on the fly when tree-profiling is invoked and call
graph will be rebuilt immediately after that which will force the
bogus target to be reachable.

Deleting the pid table after tree profiling is the way to go.


>
>> Index: value-prof.c
>> ===================================================================
>> --- value-prof.c      (revision 172721)
>> +++ value-prof.c      (working copy)
>> @@ -1083,13 +1083,35 @@ init_pid_map (void)
>>  /* Return cgraph node for function with pid */
>>
>>  static inline struct cgraph_node*
>> -find_func_by_pid (int        pid)
>> +find_func_by_pid (int pid)
>>  {
>>    init_pid_map ();
>>
>> +  if (pid >= cgraph_max_pid)
>> +    {
>> +      if (flag_profile_correction)
>> +        inform (DECL_SOURCE_LOCATION (current_function_decl),
>> +                "Inconsistent profile: indirect call target (%d) does not exist", pid);
>> +      else
>> +        error ("Inconsistent profile: indirect call target (%d) does not exist", pid);
>> +
>> +      return NULL;
>> +    }
>> +
>>    return pid_map [pid];
>
> PIDs are not dense, because functions might've been removed from before we get
> to ipa-profile.  I would suggest making pid_map VECtor to be more consistent
> with rest of GCC sourc

Why is VEC any better in terms of density ? Are you suggesting using a
hash table?


, making init_pid_map to allocate it cleared and add also
> test that VEC_index (pid_map, pid) is not NULL in addition to the bound check
> above.
>
> Also we ought to free pids once the IPA profiling pass is done. I.e. at the end of tree_profling
> function.

Yes.

Thanks,

David

>
> Honza
>

Re: FDO usability: pid handling

[Jan Hubicka]

> 
> Why is VEC any better in terms of density ? Are you suggesting using a
> hash table?
It is not any better, but we usually use VEC for variably sized arrays like
this one.  Not that I would be big fan of its API, but at least it fives
bounds checking that would catch bugs like one you are fixing more reliably.

Honza
> 
> 
> , making init_pid_map to allocate it cleared and add also
> > test that VEC_index (pid_map, pid) is not NULL in addition to the bound check
> > above.
> >
> > Also we ought to free pids once the IPA profiling pass is done. I.e. at the end of tree_profling
> > function.
> 
> Yes.
> 
> Thanks,
> 
> David
> 
> >
> > Honza
> >

Re: FDO usability: pid handling

[Xinliang David Li]

So between hashtab and VEC, which one do you prefer? Either one is fine with me.

Thanks,

David

On Tue, Apr 19, 2011 at 3:39 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>
>> Why is VEC any better in terms of density ? Are you suggesting using a
>> hash table?
> It is not any better, but we usually use VEC for variably sized arrays like
> this one.  Not that I would be big fan of its API, but at least it fives
> bounds checking that would catch bugs like one you are fixing more reliably.
>
> Honza
>>
>>
>> , making init_pid_map to allocate it cleared and add also
>> > test that VEC_index (pid_map, pid) is not NULL in addition to the bound check
>> > above.
>> >
>> > Also we ought to free pids once the IPA profiling pass is done. I.e. at the end of tree_profling
>> > function.
>>
>> Yes.
>>
>> Thanks,
>>
>> David
>>
>> >
>> > Honza
>> >
>

Re: FDO usability: pid handling

[Jan Hubicka]

> So between hashtab and VEC, which one do you prefer? Either one is fine with me.

I would go with VEC.  While the array will have holes, there are not many since
the ids are originally assigned sequentially.

Actually given that we do IPA pass now, I think you can just remove cgraph->pid
field and replace it by cgraph->uid.  The original issue was that profiling
made cgraph->uid diverge in between compilation and profling time becuase it
affected things like early inlining.  This is not true anymore, so cgraph->uid
should work well now.

Honza
> 
> Thanks,
> 
> David
> 
> On Tue, Apr 19, 2011 at 3:39 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> >>
> >> Why is VEC any better in terms of density ? Are you suggesting using a
> >> hash table?
> > It is not any better, but we usually use VEC for variably sized arrays like
> > this one.  Not that I would be big fan of its API, but at least it fives
> > bounds checking that would catch bugs like one you are fixing more reliably.
> >
> > Honza
> >>
> >>
> >> , making init_pid_map to allocate it cleared and add also
> >> > test that VEC_index (pid_map, pid) is not NULL in addition to the bound check
> >> > above.
> >> >
> >> > Also we ought to free pids once the IPA profiling pass is done. I.e. at the end of tree_profling
> >> > function.
> >>
> >> Yes.
> >>
> >> Thanks,
> >>
> >> David
> >>
> >> >
> >> > Honza
> >> >
> >

Re: FDO usability: pid handling

[Xinliang David Li]

On Tue, Apr 19, 2011 at 3:57 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> So between hashtab and VEC, which one do you prefer? Either one is fine with me.
>
> I would go with VEC.  While the array will have holes, there are not many since
> the ids are originally assigned sequentially.
>
> Actually given that we do IPA pass now, I think you can just remove cgraph->pid
> field and replace it by cgraph->uid.  The original issue was that profiling
> made cgraph->uid diverge in between compilation and profling time becuase it
> affected things like early inlining.  This is not true anymore, so cgraph->uid
> should work well now.

Why not using struct function's funcdef_no?

David

>
> Honza
>>
>> Thanks,
>>
>> David
>>
>> On Tue, Apr 19, 2011 at 3:39 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >>
>> >> Why is VEC any better in terms of density ? Are you suggesting using a
>> >> hash table?
>> > It is not any better, but we usually use VEC for variably sized arrays like
>> > this one.  Not that I would be big fan of its API, but at least it fives
>> > bounds checking that would catch bugs like one you are fixing more reliably.
>> >
>> > Honza
>> >>
>> >>
>> >> , making init_pid_map to allocate it cleared and add also
>> >> > test that VEC_index (pid_map, pid) is not NULL in addition to the bound check
>> >> > above.
>> >> >
>> >> > Also we ought to free pids once the IPA profiling pass is done. I.e. at the end of tree_profling
>> >> > function.
>> >>
>> >> Yes.
>> >>
>> >> Thanks,
>> >>
>> >> David
>> >>
>> >> >
>> >> > Honza
>> >> >
>> >
>

Re: FDO usability: pid handling

[Jan Hubicka]

> On Tue, Apr 19, 2011 at 3:57 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> >> So between hashtab and VEC, which one do you prefer? Either one is fine with me.
> >
> > I would go with VEC.  While the array will have holes, there are not many since
> > the ids are originally assigned sequentially.
> >
> > Actually given that we do IPA pass now, I think you can just remove cgraph->pid
> > field and replace it by cgraph->uid.  The original issue was that profiling
> > made cgraph->uid diverge in between compilation and profling time becuase it
> > affected things like early inlining.  This is not true anymore, so cgraph->uid
> > should work well now.
> 
> Why not using struct function's funcdef_no?

Well, we have DECL_UID, cgraph_node->uid and struct function funcdef_no.  I
think funcdef_no is least used and probably could be dropped in favour of the
other two. It seems to be used only in order to generate unique labels for functions,
that should be easilly replaceable by DECL_UID.

DECL_UID and cgraph_node->uid will probably have to stay since they drive quite
few datastructures. 

Honza

Re: FDO usability: pid handling

[Xinliang David Li]

On Tue, Apr 19, 2011 at 4:30 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On Tue, Apr 19, 2011 at 3:57 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >> So between hashtab and VEC, which one do you prefer? Either one is fine with me.
>> >
>> > I would go with VEC.  While the array will have holes, there are not many since
>> > the ids are originally assigned sequentially.
>> >
>> > Actually given that we do IPA pass now, I think you can just remove cgraph->pid
>> > field and replace it by cgraph->uid.  The original issue was that profiling
>> > made cgraph->uid diverge in between compilation and profling time becuase it
>> > affected things like early inlining.  This is not true anymore, so cgraph->uid
>> > should work well now.
>>
>> Why not using struct function's funcdef_no?
>
> Well, we have DECL_UID, cgraph_node->uid and struct function funcdef_no.  I
> think funcdef_no is least used and probably could be dropped in favour of the
> other two. It seems to be used only in order to generate unique labels for functions,
> that should be easilly replaceable by DECL_UID.
>
> DECL_UID and cgraph_node->uid will probably have to stay since they drive quite
> few datastructures.

Actually, among all the choices, funcdef_no is probably the most dense
one -- it is for function decl with definition only.  In LIPO, the
global func id is created based on module_id and funcdef_no.  IMO,
using DECL_UID or cgraph_node->uid can make the implementation
fragile.

David



>
> Honza
>

Re: FDO usability: pid handling

[Jan Hubicka]

> 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).

If we will need density later, we can have local mapping for that.

> global func id is created based on module_id and funcdef_no.  IMO,
> using DECL_UID or cgraph_node->uid can make the implementation
> fragile.

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.

What I am shooting for is to eliminate useless data in our core datastructures
to reduce memory usage and LTO streaming.  Killing funcdef_no is obviously quite
low hanging fruit here. struct_function is quite bad in this respect as it has
been a kitchen sink for various data over a decades.

But we might pretty well use it here for time being and I will eliminate it together
with the few other uses in the compiler.

Honza
> 
> David
> 
> 
> 
> >
> > Honza
> >

Re: FDO usability: pid handling

[Xinliang David Li]

On Tue, Apr 19, 2011 at 4:49 PM, Jan Hubicka <hubicka@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.

funccdef_no is pretty close to the current cgraph_node->pid given
their similarity in creation order, so it will be the safest
replacement for pid (for now).

> If we will need density later, we can have local mapping for that.
>
>> global func id is created based on module_id and funcdef_no.  IMO,
>> using DECL_UID or cgraph_node->uid can make the implementation
>> fragile.
>
> 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.

>
> What I am shooting for is to eliminate useless data in our core datastructures
> to reduce memory usage and LTO streaming.  Killing funcdef_no is obviously quite
> low hanging fruit here. struct_function is quite bad in this respect as it has
> been a kitchen sink for various data over a decades.
>
> But we might pretty well use it here for time being and I will eliminate it together
> with the few other uses in the compiler.

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;
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);
   b) it can be very useful in bug triaging via binary search by
specifying ranges of function ids (to enable optimizations etc).

Thanks,

David


>
> Honza
>>
>> David
>>
>>
>>
>> >
>> > Honza
>> >
>

Re: FDO usability: pid handling

[Jan Hubicka]

> On Tue, Apr 19, 2011 at 4:49 PM, Jan Hubicka <hubicka@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
> >> >
> >

Re: FDO usability: pid handling

[Xinliang David Li]

[-- Attachment #1: Type: text/plain, Size: 2735 bytes --]

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 <hubicka@ucw.cz> wrote:
>> On Tue, Apr 19, 2011 at 4:49 PM, Jan Hubicka <hubicka@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
>> >> >
>> >
>

[-- Attachment #2: fdo_pid2.p --]
[-- Type: text/x-pascal, Size: 7184 bytes --]

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,63 @@ 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);
+  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 +1313,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;
 }
 

Re: FDO usability: pid handling

[Xinliang David Li]

Discard this version of the patch. I have not updated source properly
and the build/test was invalid.

David

On Wed, Apr 20, 2011 at 11:22 AM, Xinliang David Li <davidxl@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 <hubicka@ucw.cz> wrote:
>>> On Tue, Apr 19, 2011 at 4:49 PM, Jan Hubicka <hubicka@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
>>> >> >
>>> >
>>
>

Re: FDO usability: pid handling

[Xinliang David Li]

[-- Attachment #1: Type: text/plain, Size: 2982 bytes --]

Please review the latest patch. SPEC2k FDO testing pass.

Thanks,

David

On Wed, Apr 20, 2011 at 11:22 AM, Xinliang David Li <davidxl@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 <hubicka@ucw.cz> wrote:
>>> On Tue, Apr 19, 2011 at 4:49 PM, Jan Hubicka <hubicka@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
>>> >> >
>>> >
>>
>

[-- Attachment #2: fdo_pid3.p --]
[-- Type: text/x-pascal, Size: 7219 bytes --]

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;
 }
 

Re: FDO usability: pid handling

[Jan Hubicka]

> @@ -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);

Given that you don't even export API for using it, I would go for init_node_map/del_node_map
in profile.h.  It is nothing generic that needs to be included into half of compiler.
>  
> -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);
You don't need wrapping struct.  In cgraph.h you already have:
DEF_VEC_P(varpool_node_ptr);
DEF_VEC_ALLOC_P(varpool_node_ptr,heap);
DEF_VEC_ALLOC_P(varpool_node_ptr,gc);
so you can use vector of cgraph_node_ptr

With those changes the patch is OK.

Re: FDO usability: pid handling

[Xinliang David Li]

On Thu, Apr 21, 2011 at 1:36 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> @@ -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);
>
> Given that you don't even export API for using it, I would go for init_node_map/del_node_map
> in profile.h.  It is nothing generic that needs to be included into half of compiler.
>>

Ok.

>> -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);
> You don't need wrapping struct.  In cgraph.h you already have:
> DEF_VEC_P(varpool_node_ptr);
> DEF_VEC_ALLOC_P(varpool_node_ptr,heap);
> DEF_VEC_ALLOC_P(varpool_node_ptr,gc);
> so you can use vector of cgraph_node_ptr
>

But ptr vector does allow me to put VEC_index as the LHS of the
assignment -- or have I missed something?

Thanks,

David


> With those changes the patch is OK.
>

Re: FDO usability: pid handling

[Jan Hubicka]

> On Thu, Apr 21, 2011 at 1:36 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> >> @@ -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);
> >
> > Given that you don't even export API for using it, I would go for init_node_map/del_node_map
> > in profile.h.  It is nothing generic that needs to be included into half of compiler.
> >>
> 
> Ok.
> 
> >> -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);
> > You don't need wrapping struct.  In cgraph.h you already have:
> > DEF_VEC_P(varpool_node_ptr);
> > DEF_VEC_ALLOC_P(varpool_node_ptr,heap);
> > DEF_VEC_ALLOC_P(varpool_node_ptr,gc);
> > so you can use vector of cgraph_node_ptr
> >
> 
> But ptr vector does allow me to put VEC_index as the LHS of the
> assignment -- or have I missed something?

If you want to set value, then use VEC_replace....

Honza

Re: FDO usability: pid handling

[Xinliang David Li]

Revised as suggested and committed.

Thanks,

David

On Thu, Apr 21, 2011 at 1:36 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> @@ -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);
>
> Given that you don't even export API for using it, I would go for init_node_map/del_node_map
> in profile.h.  It is nothing generic that needs to be included into half of compiler.
>>
>> -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);
> You don't need wrapping struct.  In cgraph.h you already have:
> DEF_VEC_P(varpool_node_ptr);
> DEF_VEC_ALLOC_P(varpool_node_ptr,heap);
> DEF_VEC_ALLOC_P(varpool_node_ptr,gc);
> so you can use vector of cgraph_node_ptr
>
> With those changes the patch is OK.
>