Re: [google][4.6] Bug fixes to function reordering linker plugin to handle local and comdat functions. (issue 5851044)

[tmsriram]

Committed to google/gcc-4_6 after validation.

On 2012/03/21 05:07:33, davidxl wrote:

ok for google branches after checkin validation.



David




http://codereview.appspot.com/5851044/

Re: [google][4.6] Bug fixes to function reordering linker plugin to handle local and comdat functions. (issue 5851044)

[davidxl]

It would be nice to add some unit/regression test cases of some sort.

David


http://codereview.appspot.com/5851044/diff/1/callgraph.c
File callgraph.c (right):

http://codereview.appspot.com/5851044/diff/1/callgraph.c#newcode309
callgraph.c:309: if (!is_prefix_of (_ZL, name))
How about static functions in namespace? How about functions in
anonymous namespace?

http://codereview.appspot.com/5851044/diff/1/callgraph.c#newcode511
callgraph.c:511: .text. };
How are the sections ordered in the array?  Keep it in mind that it is
possible to encode the actual profile count of the function in the
section name in the future.

http://codereview.appspot.com/5851044/

Re: [google][4.6] Bug fixes to function reordering linker plugin to handle local and comdat functions. (issue 5851044)

[eraman]

http://codereview.appspot.com/5851044/diff/1/callgraph.c
File callgraph.c (right):

http://codereview.appspot.com/5851044/diff/1/callgraph.c#newcode513
callgraph.c:513: const int section_priority[] = {0, 3, 4, 2, 1};
Add a comment about section_priority

http://codereview.appspot.com/5851044/diff/1/callgraph.c#newcode571
callgraph.c:571: if (section_priority[kept-section_type]
Add an example that shows why we want to do that

http://codereview.appspot.com/5851044/diff/1/callgraph.c#newcode655
callgraph.c:655: write_out_node (n_it-name, section_start[0],
section_end[0]);
In write_out_node, why take the function name and do a hash table lookup
to get the section, instead of directly passing Section_id * in the
caller. In all calls to write_out_node, you are in fact getting the name
from the Section_id *.

http://codereview.appspot.com/5851044/diff/1/callgraph.c#newcode674
callgraph.c:674: s_it-processed = 1;
setting processed to 1 is redundant as it is already done in
write_out_node.

http://codereview.appspot.com/5851044/diff/1/callgraph.h
File callgraph.h (right):

http://codereview.appspot.com/5851044/diff/1/callgraph.h#newcode31
callgraph.h:31: void push_mm_ptr (void *ptr);
push_allocated_ptr or save_allocated_ptr would be a better name.

http://codereview.appspot.com/5851044/diff/1/callgraph.h#newcode48
callgraph.h:48: push_mm_ptr (list);
It might be cleaner to create a wrapper around XNEW that calls
push_mm_ptr after XNEW. Similar for malloc as well.

http://codereview.appspot.com/5851044/

Re: [google][4.6] Bug fixes to function reordering linker plugin to handle local and comdat functions. (issue 5851044)

[tmsriram]

Uploaded new patch.

On 2012/03/20 19:25:38, davidxl wrote:

It would be nice to add some unit/regression test cases of some sort.


Made the existing unit test case check the final layout.



David



http://codereview.appspot.com/5851044/diff/1/callgraph.c
File callgraph.c (right):



http://codereview.appspot.com/5851044/diff/1/callgraph.c#newcode309
callgraph.c:309: if (!is_prefix_of (_ZL, name))
How about static functions in namespace? How about functions in

anonymous

namespace?


Thanks for pointing this out. One solution is to add more plugin
interfaces to plugin-api.h to find the section flags and the section
group. This way, comdats can be detected. All other duplicates can be
treated as file static functions. I marked this as TODO for now.

I am also thinking of sending a patch to generate unique section names
for file static functions. This will help --section-ordering-file in
gold too.


http://codereview.appspot.com/5851044/diff/1/callgraph.c#newcode511
callgraph.c:511: .text. };
How are the sections ordered in the array?  Keep it in mind that it is

possible

to encode the actual profile count of the function in the section name

in the

future.


Right, for now this parsing will work. The parsing needs to be updated
once the section names change.



http://codereview.appspot.com/5851044/

Re: [google][4.6] Bug fixes to function reordering linker plugin to handle local and comdat functions. (issue 5851044)

[tmsriram]

On 2012/03/20 19:26:26, eraman wrote:

http://codereview.appspot.com/5851044/diff/1/callgraph.c
File callgraph.c (right):



http://codereview.appspot.com/5851044/diff/1/callgraph.c#newcode513
callgraph.c:513: const int section_priority[] = {0, 3, 4, 2, 1};
Add a comment about section_priority


Done.



http://codereview.appspot.com/5851044/diff/1/callgraph.c#newcode571
callgraph.c:571: if (section_priority[kept-section_type]
Add an example that shows why we want to do that


I added a comment.



http://codereview.appspot.com/5851044/diff/1/callgraph.c#newcode655
callgraph.c:655: write_out_node (n_it-name, section_start[0],
section_end[0]);
In write_out_node, why take the function name and do a hash table

lookup to get

the section, instead of directly passing Section_id * in the caller.

In all

calls to write_out_node, you are in fact getting the name from the

Section_id *.

That is not true, I pass the Node * in the first two calls. What you
said holds for the other calls. So, I made this more efficient.



http://codereview.appspot.com/5851044/diff/1/callgraph.c#newcode674
callgraph.c:674: s_it-processed = 1;
setting processed to 1 is redundant as it is already done in

write_out_node.

Done.



http://codereview.appspot.com/5851044/diff/1/callgraph.h
File callgraph.h (right):



http://codereview.appspot.com/5851044/diff/1/callgraph.h#newcode31
callgraph.h:31: void push_mm_ptr (void *ptr);
push_allocated_ptr or save_allocated_ptr would be a better name.



Done.


http://codereview.appspot.com/5851044/diff/1/callgraph.h#newcode48
callgraph.h:48: push_mm_ptr (list);
It might be cleaner to create a wrapper around XNEW that calls

push_mm_ptr after

XNEW. Similar for malloc as well.


Done.

http://codereview.appspot.com/5851044/

Re: [google][4.6] Bug fixes to function reordering linker plugin to handle local and comdat functions. (issue 5851044)

[davidxl]

ok for google branches after checkin validation.

David

http://codereview.appspot.com/5851044/