* Re: [google][4.6] Bug fixes to function reordering linker plugin to handle local and comdat functions. (issue 5851044)
@ 2012-03-21 4:19 tmsriram
0 siblings, 0 replies; 6+ messages in thread
From: tmsriram @ 2012-03-21 4:19 UTC (permalink / raw)
To: eraman, davidxl; +Cc: gcc-patches, davidxl, reply
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, §ion_start[0],
> §ion_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/
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [google][4.6] Bug fixes to function reordering linker plugin to handle local and comdat functions. (issue 5851044)
@ 2012-03-22 1:16 tmsriram
0 siblings, 0 replies; 6+ messages in thread
From: tmsriram @ 2012-03-22 1:16 UTC (permalink / raw)
To: eraman, davidxl; +Cc: gcc-patches, davidxl, reply
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/
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [google][4.6] Bug fixes to function reordering linker plugin to handle local and comdat functions. (issue 5851044)
@ 2012-03-21 5:07 davidxl
0 siblings, 0 replies; 6+ messages in thread
From: davidxl @ 2012-03-21 5:07 UTC (permalink / raw)
To: tmsriram, eraman; +Cc: gcc-patches, reply
ok for google branches after checkin validation.
David
http://codereview.appspot.com/5851044/
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [google][4.6] Bug fixes to function reordering linker plugin to handle local and comdat functions. (issue 5851044)
@ 2012-03-21 4:18 tmsriram
0 siblings, 0 replies; 6+ messages in thread
From: tmsriram @ 2012-03-21 4:18 UTC (permalink / raw)
To: eraman, davidxl; +Cc: gcc-patches, davidxl, reply
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/
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [google][4.6] Bug fixes to function reordering linker plugin to handle local and comdat functions. (issue 5851044)
@ 2012-03-20 19:26 eraman
0 siblings, 0 replies; 6+ messages in thread
From: eraman @ 2012-03-20 19:26 UTC (permalink / raw)
To: tmsriram, davidxl; +Cc: gcc-patches, reply
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, §ion_start[0],
§ion_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/
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [google][4.6] Bug fixes to function reordering linker plugin to handle local and comdat functions. (issue 5851044)
@ 2012-03-20 19:25 davidxl
0 siblings, 0 replies; 6+ messages in thread
From: davidxl @ 2012-03-20 19:25 UTC (permalink / raw)
To: tmsriram, eraman; +Cc: gcc-patches, reply
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/
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-03-22 1:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-03-21 4:19 [google][4.6] Bug fixes to function reordering linker plugin to handle local and comdat functions. (issue 5851044) tmsriram
-- strict thread matches above, loose matches on Subject: below --
2012-03-22 1:16 tmsriram
2012-03-21 5:07 davidxl
2012-03-21 4:18 tmsriram
2012-03-20 19:26 eraman
2012-03-20 19:25 davidxl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).