LIPO based on LTO IR streaming

[davidxl, 2012-03-14 05:54:08]

Please also test existing LIPO with this changes thoroughly too (v4, v5
perflab).

David

http://codereview.appspot.com/5746044/diff/1/common.opt
File common.opt (right):

http://codereview.appspot.com/5746044/diff/1/common.opt#newcode1092
common.opt:1092: fripa2-auxiliary_module_id=
No need for 2.

http://codereview.appspot.com/5746044/diff/1/common.opt#newcode1099
common.opt:1099: int flag_ripa2 = 0
Why defining the variable in this file?

http://codereview.appspot.com/5746044/diff/1/common.opt#newcode1101
common.opt:1101: fripa2-mgf-gen=
No need for 2.

http://codereview.appspot.com/5746044/diff/1/common.opt#newcode1105
common.opt:1105: fripa2-mgf-use=
No need for 2

http://codereview.appspot.com/5746044/diff/1/common.opt#newcode2361
common.opt:2361: -gcda <file>  Set the gcov data file name.
-gcda can be confused with debug related options -gmlt, -gdwarf4 etc. It is
better to avoid the confusion. 

-fgcda

http://codereview.appspot.com/5746044/diff/1/coverage.c
File coverage.c (right):

http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode380
coverage.c:380: associated source module.  */
Add a comment here that for traditional LIPO, MODULE_ID is set to zero for
primary module.

http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode547
coverage.c:547: !module_id && !RIPA2_FE_COMP_MODE_AUX)
Is the last condition needed? Aux module reading will always have non zero
mod_id.

http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode575
coverage.c:575: if (RIPA2_FE_COMP_MODE_PRIM && ripa2_mgf_use_fname == NULL)
Refactor this into to helper function.

http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode594
coverage.c:594: fprintf(mgf_fd, "P %d %s \n", mod_info->ident,
mod_info->source_filename);
Line too long.

http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode646
coverage.c:646: {
Refactor the code into a helper function.

http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode734
coverage.c:734: current_module_id = primary_module_id;
What does this code do? It changes the default behavior?

http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode737
coverage.c:737: primary_module_exported = PRIMARY_MODULE_EXPORTED;
What does this code do?

http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode1868
coverage.c:1868: get_da_file_name (const char *base_file_name1)
change the name to orig_base_file_name.

http://codereview.appspot.com/5746044/diff/1/doc/invoke.texi
File doc/invoke.texi (right):

http://codereview.appspot.com/5746044/diff/1/doc/invoke.texi#newcode8150
doc/invoke.texi:8150: @code{streaming} is a implementation that based on LTO
streaming technology.
a->an

http://codereview.appspot.com/5746044/diff/1/doc/invoke.texi#newcode8150
doc/invoke.texi:8150: @code{streaming} is a implementation that based on LTO
streaming technology.
that --> that is

http://codereview.appspot.com/5746044/diff/1/doc/invoke.texi#newcode8200
doc/invoke.texi:8200: @item fripa2-auxiliary_module_id
Please remove '2'

http://codereview.appspot.com/5746044/diff/1/gcc.c
File gcc.c (right):

http://codereview.appspot.com/5746044/diff/1/gcc.c#newcode2571
gcc.c:2571: {
Helper function please

http://codereview.appspot.com/5746044/diff/1/ipa.c
File ipa.c (right):

http://codereview.appspot.com/5746044/diff/1/ipa.c#newcode677
ipa.c:677: /* Return true if NODE needs special handling in ripa2 mode.  */
what special handling?  ripa2 mode is also very vague. Please make the comments
clearer and the function name more meaningful.

http://codereview.appspot.com/5746044/diff/1/ipa.c#newcode704
ipa.c:704: {
Is this for promoted symbol only?  Please explain in more details.

http://codereview.appspot.com/5746044/diff/1/ipa.c#newcode758
ipa.c:758: issue.  */
Can you file a bug upstream and propose a separate fix?

http://codereview.appspot.com/5746044/diff/1/ipa.c#newcode799
ipa.c:799: return (RIPA2_IN_LTO_P &&
Have comments explaining the logic below.

http://codereview.appspot.com/5746044/diff/1/ipa.c#newcode901
ipa.c:901: fprintf(stderr, "Dissolve comdat symbol: %s\n",
--> inform ().

http://codereview.appspot.com/5746044/diff/1/l-ipo.c
File l-ipo.c (right):

http://codereview.appspot.com/5746044/diff/1/l-ipo.c#newcode1791
l-ipo.c:1791: fprintf(stderr, "Promote func %s to global (%d): ",
fprintf --> inform?

http://codereview.appspot.com/5746044/diff/1/l-ipo.c#newcode1791
l-ipo.c:1791: fprintf(stderr, "Promote func %s to global (%d): ",
func --> function

http://codereview.appspot.com/5746044/diff/1/l-ipo.c#newcode1804
l-ipo.c:1804: fprintf(stderr, "Promote var %s to global (%d): ",
printf --> inform ?

http://codereview.appspot.com/5746044/diff/1/l-ipo.c#newcode1867
l-ipo.c:1867: if (primary_module_id && primary_module_exported && !TREE_PUBLIC
(decl))
This changes the default LIPO behavior.

http://codereview.appspot.com/5746044/diff/1/l-ipo.h
File l-ipo.h (right):

http://codereview.appspot.com/5746044/diff/1/l-ipo.h#newcode35
l-ipo.h:35: #define RIPA2_COMP_MODE ((current_module_id != 0 \
L_IPO_STREAM_COMP_MODE

http://codereview.appspot.com/5746044/diff/1/toplev.c
File toplev.c (right):

http://codereview.appspot.com/5746044/diff/1/toplev.c#newcode622
toplev.c:622: {
Guarding main piece of code in the default path like this does not look good. 

For this comp mode, should the function just return?

http://codereview.appspot.com/5746044/diff/1/toplev.c#newcode1954
toplev.c:1954: if (!in_lto_p)
Is this a bug in LTO? Should be split out and submit upstream.

http://codereview.appspot.com/5746044/diff/1/toplev.c#newcode2022
toplev.c:2022: ripa2 compilation mode.  */
Please comment in more detail what the code tries to do.

http://codereview.appspot.com/5746044/diff/1/toplev.c#newcode2024
toplev.c:2024: {
Fix indentation.

[xur, 2012-03-14 17:36:26]

Should I replace ripa2 to ripa in all files?

Will send patchset 2 soon.

-Rong

http://codereview.appspot.com/5746044/diff/1/common.opt
File common.opt (right):

http://codereview.appspot.com/5746044/diff/1/common.opt#newcode1092
common.opt:1092: fripa2-auxiliary_module_id=
On 2012/03/14 05:54:09, davidxl wrote:
> No need for 2.
Done.

http://codereview.appspot.com/5746044/diff/1/common.opt#newcode1099
common.opt:1099: int flag_ripa2 = 0
On 2012/03/14 05:54:09, davidxl wrote:
> Why defining the variable in this file?
common.def will be processed and flag variables are automatically defined
through this pre-processing. I thought it's better to have all the flag related
variables in one place. But this can be changed.

http://codereview.appspot.com/5746044/diff/1/common.opt#newcode1101
common.opt:1101: fripa2-mgf-gen=
On 2012/03/14 05:54:09, davidxl wrote:
> No need for 2.
Done

http://codereview.appspot.com/5746044/diff/1/common.opt#newcode1105
common.opt:1105: fripa2-mgf-use=
On 2012/03/14 05:54:09, davidxl wrote:
> No need for 2
Done.

http://codereview.appspot.com/5746044/diff/1/common.opt#newcode2361
common.opt:2361: -gcda <file>  Set the gcov data file name.
On 2012/03/14 05:54:09, davidxl wrote:
> -gcda can be confused with debug related options -gmlt, -gdwarf4 etc. It is
> better to avoid the confusion. 
> 
> -fgcda
Done. 

PS: This option may not be needed. Initially I used auxbase for the same
purpose, but I had some difficulty to handle the gcda file for aux module in
different sub-directory.

http://codereview.appspot.com/5746044/diff/1/coverage.c
File coverage.c (right):

http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode380
coverage.c:380: associated source module.  */
On 2012/03/14 05:54:09, davidxl wrote:
> Add a comment here that for traditional LIPO, MODULE_ID is set to zero for
> primary module.
will do

http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode547
coverage.c:547: !module_id && !RIPA2_FE_COMP_MODE_AUX)
On 2012/03/14 05:54:09, davidxl wrote:
> Is the last condition needed? Aux module reading will always have non zero
> mod_id.

you are right. it's not needed. Will change.

http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode575
coverage.c:575: if (RIPA2_FE_COMP_MODE_PRIM && ripa2_mgf_use_fname == NULL)
On 2012/03/14 05:54:09, davidxl wrote:
> Refactor this into to helper function.

Will be done.

http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode594
coverage.c:594: fprintf(mgf_fd, "P %d %s \n", mod_info->ident,
mod_info->source_filename);
On 2012/03/14 05:54:09, davidxl wrote:
> Line too long.
will fix.

http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode646
coverage.c:646: {
On 2012/03/14 05:54:09, davidxl wrote:
> Refactor the code into a helper function.

will be done.

http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode734
coverage.c:734: current_module_id = primary_module_id;
On 2012/03/14 05:54:09, davidxl wrote:
> What does this code do? It changes the default behavior?
In original LIPO, current_module_id is a temp being pushed and popped based on
the module being processed. 
but here, the compilation is separated in different cc1 invocation. I need to
set current_module_id here (upon reading the gcda file).

I don't think it actually changes the default behavior as current_module_id will
be reset.

http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode737
coverage.c:737: primary_module_exported = PRIMARY_MODULE_EXPORTED;
On 2012/03/14 05:54:09, davidxl wrote:
> What does this code do?
I need to save streaming out boolean PRIMARY_MODULE_EXPORTED. So I have a global
variable to save this boolean. Foa aux_module, it's always 0.

http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode1868
coverage.c:1868: get_da_file_name (const char *base_file_name1)
On 2012/03/14 05:54:09, davidxl wrote:
> change the name to orig_base_file_name.

will do.

http://codereview.appspot.com/5746044/diff/1/doc/invoke.texi
File doc/invoke.texi (right):

http://codereview.appspot.com/5746044/diff/1/doc/invoke.texi#newcode8150
doc/invoke.texi:8150: @code{streaming} is a implementation that based on LTO
streaming technology.
On 2012/03/14 05:54:09, davidxl wrote:
> that --> that is
fixed

http://codereview.appspot.com/5746044/diff/1/doc/invoke.texi#newcode8150
doc/invoke.texi:8150: @code{streaming} is a implementation that based on LTO
streaming technology.
On 2012/03/14 05:54:09, davidxl wrote:
> a->an
fixed

http://codereview.appspot.com/5746044/diff/1/doc/invoke.texi#newcode8200
doc/invoke.texi:8200: @item fripa2-auxiliary_module_id
On 2012/03/14 05:54:09, davidxl wrote:
> Please remove '2'
will do

http://codereview.appspot.com/5746044/diff/1/gcc.c
File gcc.c (right):

http://codereview.appspot.com/5746044/diff/1/gcc.c#newcode2571
gcc.c:2571: {
On 2012/03/14 05:54:09, davidxl wrote:
> Helper function please

will do.

And I will change ripa2 to ripa in this file.

http://codereview.appspot.com/5746044/diff/1/ipa.c
File ipa.c (right):

http://codereview.appspot.com/5746044/diff/1/ipa.c#newcode677
ipa.c:677: /* Return true if NODE needs special handling in ripa2 mode.  */
On 2012/03/14 05:54:09, davidxl wrote:
> what special handling?  ripa2 mode is also very vague. Please make the
comments
> clearer and the function name more meaningful.
will cgraph_node_lipo_maybe_referenced_in_other_module better? 
We should not mix it with lto's cgraph attributes like: in_other_partition or
reachable_from_other_partition.

http://codereview.appspot.com/5746044/diff/1/ipa.c#newcode704
ipa.c:704: {
On 2012/03/14 05:54:09, davidxl wrote:
> Is this for promoted symbol only?  Please explain in more details.
it's for all the symbols in the aux modules. if that symbol is already global,
we don't promote it, but the visibility need to be the same here.

http://codereview.appspot.com/5746044/diff/1/ipa.c#newcode758
ipa.c:758: issue.  */
On 2012/03/14 05:54:09, davidxl wrote:
> Can you file a bug upstream and propose a separate fix?

will do this in parallel.

http://codereview.appspot.com/5746044/diff/1/ipa.c#newcode799
ipa.c:799: return (RIPA2_IN_LTO_P &&
On 2012/03/14 05:54:09, davidxl wrote:
> Have comments explaining the logic below.
will add.

http://codereview.appspot.com/5746044/diff/1/ipa.c#newcode901
ipa.c:901: fprintf(stderr, "Dissolve comdat symbol: %s\n",
On 2012/03/14 05:54:09, davidxl wrote:
> --> inform ().
will change.

http://codereview.appspot.com/5746044/diff/1/l-ipo.c
File l-ipo.c (right):

http://codereview.appspot.com/5746044/diff/1/l-ipo.c#newcode1791
l-ipo.c:1791: fprintf(stderr, "Promote func %s to global (%d): ",
On 2012/03/14 05:54:09, davidxl wrote:
> fprintf --> inform?

will fix.

http://codereview.appspot.com/5746044/diff/1/l-ipo.c#newcode1791
l-ipo.c:1791: fprintf(stderr, "Promote func %s to global (%d): ",
On 2012/03/14 05:54:09, davidxl wrote:
> fprintf --> inform?
will fix

http://codereview.appspot.com/5746044/diff/1/l-ipo.c#newcode1804
l-ipo.c:1804: fprintf(stderr, "Promote var %s to global (%d): ",
On 2012/03/14 05:54:09, davidxl wrote:
> printf --> inform ?
will fix.

http://codereview.appspot.com/5746044/diff/1/l-ipo.c#newcode1867
l-ipo.c:1867: if (primary_module_id && primary_module_exported && !TREE_PUBLIC
(decl))
On 2012/03/14 05:54:09, davidxl wrote:
> This changes the default LIPO behavior.

I think it will not:
(1) in default LIPO, primary_module_id will never be 0 here. The promotion is
guarded by L_IPO_COMP_MODE which checks primary_module_id.
(2) primary_module_exported is the same as PRIMARY_MODULE_EXPORTED.

PS: in streaming lipo, we use to module_id:primary_module_id and
current_module_id. primary_module_id can be 0.

http://codereview.appspot.com/5746044/diff/1/l-ipo.h
File l-ipo.h (right):

http://codereview.appspot.com/5746044/diff/1/l-ipo.h#newcode35
l-ipo.h:35: #define RIPA2_COMP_MODE ((current_module_id != 0 \
On 2012/03/14 05:54:09, davidxl wrote:
> L_IPO_STREAM_COMP_MODE

will change.

http://codereview.appspot.com/5746044/diff/1/toplev.c
File toplev.c (right):

http://codereview.appspot.com/5746044/diff/1/toplev.c#newcode622
toplev.c:622: {
On 2012/03/14 05:54:09, davidxl wrote:
> Guarding main piece of code in the default path like this does not look good. 
> 
> For this comp mode, should the function just return?

it needs some emit some markers for lto. Note the code guarded by
flag_generate_lto will be executed.

http://codereview.appspot.com/5746044/diff/1/toplev.c#newcode1954
toplev.c:1954: if (!in_lto_p)
On 2012/03/14 05:54:09, davidxl wrote:
> Is this a bug in LTO? Should be split out and submit upstream.
will look this again.

http://codereview.appspot.com/5746044/diff/1/toplev.c#newcode2022
toplev.c:2022: ripa2 compilation mode.  */
On 2012/03/14 05:54:09, davidxl wrote:
> Please comment in more detail what the code tries to do.
will do.

http://codereview.appspot.com/5746044/diff/1/toplev.c#newcode2024
toplev.c:2024: {
On 2012/03/14 05:54:09, davidxl wrote:
> Fix indentation.

will do.

[davidxl, 2012-03-14 17:42:03]

On Wed, Mar 14, 2012 at 10:36 AM,  <xur@google.com> wrote:
> Reviewers: davidxl,
>
> Message:
> Should I replace ripa2 to ripa in all files?

Or ripa_stream_.... to make it more explicit.

David

>
> Will send patchset 2 soon.
>
> -Rong
>
>
>
> http://codereview.appspot.com/5746044/diff/1/common.opt
> File common.opt (right):
>
> http://codereview.appspot.com/5746044/diff/1/common.opt#newcode1092
> common.opt:1092: fripa2-auxiliary_module_id=
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> No need for 2.
>
> Done.
>
>
> http://codereview.appspot.com/5746044/diff/1/common.opt#newcode1099
> common.opt:1099: int flag_ripa2 = 0
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> Why defining the variable in this file?
>
> common.def will be processed and flag variables are automatically
> defined through this pre-processing. I thought it's better to have all
> the flag related variables in one place. But this can be changed.
>
>
> http://codereview.appspot.com/5746044/diff/1/common.opt#newcode1101
> common.opt:1101: fripa2-mgf-gen=
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> No need for 2.
>
> Done
>
>
> http://codereview.appspot.com/5746044/diff/1/common.opt#newcode1105
> common.opt:1105: fripa2-mgf-use=
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> No need for 2
>
> Done.
>
>
> http://codereview.appspot.com/5746044/diff/1/common.opt#newcode2361
> common.opt:2361: -gcda <file>  Set the gcov data file name.
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> -gcda can be confused with debug related options -gmlt, -gdwarf4 etc.
>
> It is
>>
>> better to avoid the confusion.
>
>
>> -fgcda
>
> Done.
>
> PS: This option may not be needed. Initially I used auxbase for the same
> purpose, but I had some difficulty to handle the gcda file for aux
> module in different sub-directory.
>
>
> http://codereview.appspot.com/5746044/diff/1/coverage.c
> File coverage.c (right):
>
> http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode380
> coverage.c:380: associated source module.  */
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> Add a comment here that for traditional LIPO, MODULE_ID is set to zero
>
> for
>>
>> primary module.
>
> will do
>
>
> http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode547
> coverage.c:547: !module_id && !RIPA2_FE_COMP_MODE_AUX)
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> Is the last condition needed? Aux module reading will always have non
>
> zero
>>
>> mod_id.
>
>
> you are right. it's not needed. Will change.
>
>
> http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode575
> coverage.c:575: if (RIPA2_FE_COMP_MODE_PRIM && ripa2_mgf_use_fname ==
> NULL)
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> Refactor this into to helper function.
>
>
> Will be done.
>
>
> http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode594
> coverage.c:594: fprintf(mgf_fd, "P %d %s \n", mod_info->ident,
> mod_info->source_filename);
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> Line too long.
>
> will fix.
>
> http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode646
> coverage.c:646: {
>
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> Refactor the code into a helper function.
>
>
> will be done.
>
>
> http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode734
> coverage.c:734: current_module_id = primary_module_id;
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> What does this code do? It changes the default behavior?
>
> In original LIPO, current_module_id is a temp being pushed and popped
> based on the module being processed.
> but here, the compilation is separated in different cc1 invocation. I
> need to set current_module_id here (upon reading the gcda file).
>
> I don't think it actually changes the default behavior as
> current_module_id will be reset.
>
>
> http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode737
> coverage.c:737: primary_module_exported = PRIMARY_MODULE_EXPORTED;
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> What does this code do?
>
> I need to save streaming out boolean PRIMARY_MODULE_EXPORTED. So I have
> a global variable to save this boolean. Foa aux_module, it's always 0.
>
>
> http://codereview.appspot.com/5746044/diff/1/coverage.c#newcode1868
> coverage.c:1868: get_da_file_name (const char *base_file_name1)
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> change the name to orig_base_file_name.
>
>
> will do.
>
>
> http://codereview.appspot.com/5746044/diff/1/doc/invoke.texi
> File doc/invoke.texi (right):
>
> http://codereview.appspot.com/5746044/diff/1/doc/invoke.texi#newcode8150
> doc/invoke.texi:8150: @code{streaming} is a implementation that based on
> LTO streaming technology.
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> that --> that is
>
> fixed
>
>
> http://codereview.appspot.com/5746044/diff/1/doc/invoke.texi#newcode8150
> doc/invoke.texi:8150: @code{streaming} is a implementation that based on
> LTO streaming technology.
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> a->an
>
> fixed
>
>
> http://codereview.appspot.com/5746044/diff/1/doc/invoke.texi#newcode8200
> doc/invoke.texi:8200: @item fripa2-auxiliary_module_id
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> Please remove '2'
>
> will do
>
>
> http://codereview.appspot.com/5746044/diff/1/gcc.c
> File gcc.c (right):
>
> http://codereview.appspot.com/5746044/diff/1/gcc.c#newcode2571
> gcc.c:2571: {
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> Helper function please
>
>
> will do.
>
> And I will change ripa2 to ripa in this file.
>
>
> http://codereview.appspot.com/5746044/diff/1/ipa.c
> File ipa.c (right):
>
> http://codereview.appspot.com/5746044/diff/1/ipa.c#newcode677
> ipa.c:677: /* Return true if NODE needs special handling in ripa2 mode.
> */
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> what special handling?  ripa2 mode is also very vague. Please make the
>
> comments
>>
>> clearer and the function name more meaningful.
>
> will cgraph_node_lipo_maybe_referenced_in_other_module better?
> We should not mix it with lto's cgraph attributes like:
> in_other_partition or reachable_from_other_partition.
>
> http://codereview.appspot.com/5746044/diff/1/ipa.c#newcode704
> ipa.c:704: {
>
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> Is this for promoted symbol only?  Please explain in more details.
>
> it's for all the symbols in the aux modules. if that symbol is already
> global, we don't promote it, but the visibility need to be the same
> here.
>
>
> http://codereview.appspot.com/5746044/diff/1/ipa.c#newcode758
> ipa.c:758: issue.  */
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> Can you file a bug upstream and propose a separate fix?
>
>
> will do this in parallel.
>
>
> http://codereview.appspot.com/5746044/diff/1/ipa.c#newcode799
> ipa.c:799: return (RIPA2_IN_LTO_P &&
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> Have comments explaining the logic below.
>
> will add.
>
>
> http://codereview.appspot.com/5746044/diff/1/ipa.c#newcode901
> ipa.c:901: fprintf(stderr, "Dissolve comdat symbol: %s\n",
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> --> inform ().
>
> will change.
>
>
> http://codereview.appspot.com/5746044/diff/1/l-ipo.c
> File l-ipo.c (right):
>
> http://codereview.appspot.com/5746044/diff/1/l-ipo.c#newcode1791
> l-ipo.c:1791: fprintf(stderr, "Promote func %s to global (%d): ",
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> fprintf --> inform?
>
>
> will fix.
>
>
> http://codereview.appspot.com/5746044/diff/1/l-ipo.c#newcode1791
> l-ipo.c:1791: fprintf(stderr, "Promote func %s to global (%d): ",
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> fprintf --> inform?
>
> will fix
>
>
> http://codereview.appspot.com/5746044/diff/1/l-ipo.c#newcode1804
> l-ipo.c:1804: fprintf(stderr, "Promote var %s to global (%d): ",
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> printf --> inform ?
>
> will fix.
>
>
> http://codereview.appspot.com/5746044/diff/1/l-ipo.c#newcode1867
> l-ipo.c:1867: if (primary_module_id && primary_module_exported &&
> !TREE_PUBLIC (decl))
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> This changes the default LIPO behavior.
>
>
> I think it will not:
> (1) in default LIPO, primary_module_id will never be 0 here. The
> promotion is guarded by L_IPO_COMP_MODE which checks primary_module_id.
> (2) primary_module_exported is the same as PRIMARY_MODULE_EXPORTED.
>
> PS: in streaming lipo, we use to module_id:primary_module_id and
> current_module_id. primary_module_id can be 0.
>
>
> http://codereview.appspot.com/5746044/diff/1/l-ipo.h
> File l-ipo.h (right):
>
> http://codereview.appspot.com/5746044/diff/1/l-ipo.h#newcode35
> l-ipo.h:35: #define RIPA2_COMP_MODE ((current_module_id != 0 \
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> L_IPO_STREAM_COMP_MODE
>
>
> will change.
>
>
> http://codereview.appspot.com/5746044/diff/1/toplev.c
> File toplev.c (right):
>
> http://codereview.appspot.com/5746044/diff/1/toplev.c#newcode622
> toplev.c:622: {
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> Guarding main piece of code in the default path like this does not
>
> look good.
>
>> For this comp mode, should the function just return?
>
>
> it needs some emit some markers for lto. Note the code guarded by
> flag_generate_lto will be executed.
>
>
> http://codereview.appspot.com/5746044/diff/1/toplev.c#newcode1954
> toplev.c:1954: if (!in_lto_p)
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> Is this a bug in LTO? Should be split out and submit upstream.
>
> will look this again.
>
>
> http://codereview.appspot.com/5746044/diff/1/toplev.c#newcode2022
> toplev.c:2022: ripa2 compilation mode.  */
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> Please comment in more detail what the code tries to do.
>
> will do.
>
> http://codereview.appspot.com/5746044/diff/1/toplev.c#newcode2024
> toplev.c:2024: {
> On 2012/03/14 05:54:09, davidxl wrote:
>>
>> Fix indentation.
>
>
> will do.
>
>
>
> Please review this at http://codereview.appspot.com/5746044/
>
> Affected files:
>  M     cgraph.c
>  M     cgraphunit.c
>  M     common.opt
>  M     coverage.c
>  M     cp/lang-specs.h
>  M     doc/invoke.texi
>  M     fortran/lang-specs.h
>  M     gcc.c
>  M     ipa.c
>  M     l-ipo.c
>  M     l-ipo.h
>  M     lto-cgraph.c
>  M     lto-section-in.c
>  M     lto-streamer-in.c
>  M     lto-streamer-out.c
>  M     lto-streamer.c
>  M     lto-streamer.h
>  M     lto-symtab.c
>  M     lto/lto.c
>  M     opts.c
>  M     toplev.c
>  M     tree-sra.c
>
>

[davidxl, 2012-03-14 21:09:05]

http://codereview.appspot.com/5746044/diff/1/common.opt
File common.opt (right):

http://codereview.appspot.com/5746044/diff/1/common.opt#newcode1099
common.opt:1099: int flag_ripa2 = 0
It is fine to keep it here.

On 2012/03/14 17:36:26, xur wrote:
> On 2012/03/14 05:54:09, davidxl wrote:
> > Why defining the variable in this file?
> common.def will be processed and flag variables are automatically defined
> through this pre-processing. I thought it's better to have all the flag
related
> variables in one place. But this can be changed.

http://codereview.appspot.com/5746044/diff/1/ipa.c
File ipa.c (right):

http://codereview.appspot.com/5746044/diff/1/ipa.c#newcode677
ipa.c:677: /* Return true if NODE needs special handling in ripa2 mode.  */
can_have_cross_module_ref_in_lipo_p (..)

On 2012/03/14 17:36:26, xur wrote:
> On 2012/03/14 05:54:09, davidxl wrote:
> > what special handling?  ripa2 mode is also very vague. Please make the
> comments
> > clearer and the function name more meaningful.
> will cgraph_node_lipo_maybe_referenced_in_other_module better? 
> We should not mix it with lto's cgraph attributes like: in_other_partition or
> reachable_from_other_partition.

http://codereview.appspot.com/5746044/diff/1/ipa.c#newcode704
ipa.c:704: {
The comment before the function body does not match what is described in your
reply. Need to make the function comment better.

On 2012/03/14 17:36:26, xur wrote:
> On 2012/03/14 05:54:09, davidxl wrote:
> > Is this for promoted symbol only?  Please explain in more details.
> it's for all the symbols in the aux modules. if that symbol is already global,
> we don't promote it, but the visibility need to be the same here.

http://codereview.appspot.com/5746044/diff/1/l-ipo.c
File l-ipo.c (right):

http://codereview.appspot.com/5746044/diff/1/l-ipo.c#newcode1867
l-ipo.c:1867: if (primary_module_id && primary_module_exported && !TREE_PUBLIC
(decl))
Add  some assertion here?

On 2012/03/14 17:36:26, xur wrote:
> On 2012/03/14 05:54:09, davidxl wrote:
> > This changes the default LIPO behavior.
> 
> I think it will not:
> (1) in default LIPO, primary_module_id will never be 0 here. The promotion is
> guarded by L_IPO_COMP_MODE which checks primary_module_id.
> (2) primary_module_exported is the same as PRIMARY_MODULE_EXPORTED.
> 
> PS: in streaming lipo, we use to module_id:primary_module_id and
> current_module_id. primary_module_id can be 0.

http://codereview.appspot.com/5746044/diff/1/toplev.c
File toplev.c (right):

http://codereview.appspot.com/5746044/diff/1/toplev.c#newcode622
toplev.c:622: {
Perhaps refactor the lto_marker code into a helper function, and clone the calls
to it for RIPA path.

On 2012/03/14 17:36:26, xur wrote:
> On 2012/03/14 05:54:09, davidxl wrote:
> > Guarding main piece of code in the default path like this does not look
good. 
> > 
> > For this comp mode, should the function just return?
> 
> it needs some emit some markers for lto. Note the code guarded by
> flag_generate_lto will be executed.

[davidxl, 2012-03-14 21:50:30]

http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c
File lto-streamer-in.c (right):

http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c#newcode2698
lto-streamer-in.c:2698: if (TREE_CODE (result) == VAR_DECL)
Is this change for LTO too?

http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c#newcode2700
lto-streamer-in.c:2700: /* This can be called in two places: (1) reading decls,
(2)
Fix indentation.

http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c#newcode2709
lto-streamer-in.c:2709: {
Same here.

http://codereview.appspot.com/5746044/diff/1/lto-streamer-out.c
File lto-streamer-out.c (right):

http://codereview.appspot.com/5746044/diff/1/lto-streamer-out.c#newcode2648
lto-streamer-out.c:2648: unsigned char module_type = 0;
Not sure if using nested scope is the right style. Using a helper function may
be better.

http://codereview.appspot.com/5746044/diff/1/lto-symtab.c
File lto-symtab.c (right):

http://codereview.appspot.com/5746044/diff/1/lto-symtab.c#newcode153
lto-symtab.c:153: (2) have readonly static scope.
TREE_STATIC means the variable has definition -- so the comment is not quite
right.  The real reason is that aux module global vars will be externalized, but
the iniitialzer is still kept to enable const propagation.

[xur, 2012-03-14 23:12:53]

replies are inlined.

On Wed, Mar 14, 2012 at 2:50 PM, <davidxl@google.com> wrote:

>
>
http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**in.c<http://cod...
> File lto-streamer-in.c (right):
>
> http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**
>
in.c#newcode2698<http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c#newcode2698>
> lto-streamer-in.c:2698: if (TREE_CODE (result) == VAR_DECL)
> Is this change for LTO too?
>
> yes this affect LTO too. I did not guard it because I think this is a LTO
bug. I will file a bug in upstream.
I can guard the code with flag_ripa_stream (also need to change another
place in lto.c).
But I don't think anybody cares -- nobody is really use lto in google-4_6
branch.


> http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**
>
in.c#newcode2700<http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c#newcode2700>
> lto-streamer-in.c:2700: /* This can be called in two places: (1) reading
> decls, (2)
> Fix indentation.
>
> Done.


> http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**
>
in.c#newcode2709<http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c#newcode2709>
> lto-streamer-in.c:2709: {
> Same here.
>
> Done.


>
http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**out.c<http://co...
> File lto-streamer-out.c (right):
>
> http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**
>
out.c#newcode2648<http://codereview.appspot.com/5746044/diff/1/lto-streamer-out.c#newcode2648>
> lto-streamer-out.c:2648: unsigned char module_type = 0;
> Not sure if using nested scope is the right style. Using a helper
> function may be better.
>
> Will do.


>
http://codereview.appspot.com/**5746044/diff/1/lto-symtab.c<http://codereview...
> File lto-symtab.c (right):
>
>
http://codereview.appspot.com/**5746044/diff/1/lto-symtab.c#**newcode153<http...
> lto-symtab.c:153: (2) have readonly static scope.
> TREE_STATIC means the variable has definition -- so the comment is not
> quite right.  The real reason is that aux module global vars will be
> externalized, but the iniitialzer is still kept to enable const
> propagation.
>
> you are probably right, I have that comment since the very beginning of
the project.
I'll double check and change it according.


>
http://codereview.appspot.com/**5746044/<http://codereview.appspot.com/5746044/>
>

[xur, 2012-03-15 18:16:20]

I sent out patchset 2 to
http://codereview.appspot.com/5746044/

It incorporates David's feedbacks and suggestions. Some of the LTO
bug fixes are still included to make the patch complete.

Tested with bootstrap.

Test this new lipo (-fripa=streaming) with SPEC2000, SPEC2006. This time I
included all Fortran programs.
178.galgel and 187.facerec failed at profile-gen compilation.
447.dealII failed due to source problem.
459.GemsFDTD failed in profile-gen compilation.
481.wrf failed at run time of optimize binary (I will take a look at this).

Perflab testing (existing lipo and this new lipo) is undergoing.

-Rong


On Wed, Mar 14, 2012 at 4:12 PM, Rong Xu <xur@google.com> wrote:

> replies are inlined.
>
> On Wed, Mar 14, 2012 at 2:50 PM, <davidxl@google.com> wrote:
>
>>
>>
http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**in.c<http://cod...
>> File lto-streamer-in.c (right):
>>
>> http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**
>>
in.c#newcode2698<http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c#newcode2698>
>> lto-streamer-in.c:2698: if (TREE_CODE (result) == VAR_DECL)
>> Is this change for LTO too?
>>
>> yes this affect LTO too. I did not guard it because I think this is a LTO
> bug. I will file a bug in upstream.
> I can guard the code with flag_ripa_stream (also need to change another
> place in lto.c).
> But I don't think anybody cares -- nobody is really use lto in google-4_6
> branch.
>
>
>> http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**
>>
in.c#newcode2700<http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c#newcode2700>
>> lto-streamer-in.c:2700: /* This can be called in two places: (1) reading
>> decls, (2)
>> Fix indentation.
>>
>> Done.
>
>
>> http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**
>>
in.c#newcode2709<http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c#newcode2709>
>> lto-streamer-in.c:2709: {
>> Same here.
>>
>> Done.
>
>
>>
http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**out.c<http://co...
>> File lto-streamer-out.c (right):
>>
>> http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**
>>
out.c#newcode2648<http://codereview.appspot.com/5746044/diff/1/lto-streamer-out.c#newcode2648>
>> lto-streamer-out.c:2648: unsigned char module_type = 0;
>> Not sure if using nested scope is the right style. Using a helper
>> function may be better.
>>
>> Will do.
>
>
>>
http://codereview.appspot.com/**5746044/diff/1/lto-symtab.c<http://codereview...
>> File lto-symtab.c (right):
>>
>>
http://codereview.appspot.com/**5746044/diff/1/lto-symtab.c#**newcode153<http...
>> lto-symtab.c:153: (2) have readonly static scope.
>> TREE_STATIC means the variable has definition -- so the comment is not
>> quite right.  The real reason is that aux module global vars will be
>> externalized, but the iniitialzer is still kept to enable const
>> propagation.
>>
>> you are probably right, I have that comment since the very beginning of
> the project.
> I'll double check and change it according.
>
>
>>
http://codereview.appspot.com/**5746044/<http://codereview.appspot.com/5746044/>
>>
>
>

[xur, 2012-03-15 20:24:45]

On Thu, Mar 15, 2012 at 11:16 AM, Rong Xu <xur@google.com> wrote:

> I sent out patchset 2 to
> http://codereview.appspot.com/5746044/
>
> It incorporates David's feedbacks and suggestions. Some of the LTO
> bug fixes are still included to make the patch complete.
>
> Tested with bootstrap.
>
> Test this new lipo (-fripa=streaming) with SPEC2000, SPEC2006. This time I
> included all Fortran programs.
> 178.galgel and 187.facerec failed at profile-gen compilation.
>

178.galgel and 187.facerec passed with manually rerun. It's either a
perflab setup issue or glitch.


> 447.dealII failed due to source problem.
> 459.GemsFDTD failed in profile-gen compilation.
> 481.wrf failed at run time of optimize binary (I will take a look at this).
>

459.GemsFDTD fails to build with -O2
481.wrf has run-time error with -O2.
So they are not related to this patch.


>
> Perflab testing (existing lipo and this new lipo) is undergoing.
>
> -Rong
>
>
> On Wed, Mar 14, 2012 at 4:12 PM, Rong Xu <xur@google.com> wrote:
>
>> replies are inlined.
>>
>> On Wed, Mar 14, 2012 at 2:50 PM, <davidxl@google.com> wrote:
>>
>>>
>>>
http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**in.c<http://cod...
>>> File lto-streamer-in.c (right):
>>>
>>> http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**
>>>
in.c#newcode2698<http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c#newcode2698>
>>> lto-streamer-in.c:2698: if (TREE_CODE (result) == VAR_DECL)
>>> Is this change for LTO too?
>>>
>>> yes this affect LTO too. I did not guard it because I think this is a
>> LTO bug. I will file a bug in upstream.
>> I can guard the code with flag_ripa_stream (also need to change another
>> place in lto.c).
>> But I don't think anybody cares -- nobody is really use lto in google-4_6
>> branch.
>>
>>
>>> http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**
>>>
in.c#newcode2700<http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c#newcode2700>
>>> lto-streamer-in.c:2700: /* This can be called in two places: (1) reading
>>> decls, (2)
>>> Fix indentation.
>>>
>>> Done.
>>
>>
>>> http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**
>>>
in.c#newcode2709<http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c#newcode2709>
>>> lto-streamer-in.c:2709: {
>>> Same here.
>>>
>>> Done.
>>
>>
>>>
http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**out.c<http://co...
>>> File lto-streamer-out.c (right):
>>>
>>> http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**
>>>
out.c#newcode2648<http://codereview.appspot.com/5746044/diff/1/lto-streamer-out.c#newcode2648>
>>> lto-streamer-out.c:2648: unsigned char module_type = 0;
>>> Not sure if using nested scope is the right style. Using a helper
>>> function may be better.
>>>
>>> Will do.
>>
>>
>>>
http://codereview.appspot.com/**5746044/diff/1/lto-symtab.c<http://codereview...
>>> File lto-symtab.c (right):
>>>
>>>
http://codereview.appspot.com/**5746044/diff/1/lto-symtab.c#**newcode153<http...
>>> lto-symtab.c:153: (2) have readonly static scope.
>>> TREE_STATIC means the variable has definition -- so the comment is not
>>> quite right.  The real reason is that aux module global vars will be
>>> externalized, but the iniitialzer is still kept to enable const
>>> propagation.
>>>
>>> you are probably right, I have that comment since the very beginning of
>> the project.
>> I'll double check and change it according.
>>
>>
>>>
http://codereview.appspot.com/**5746044/<http://codereview.appspot.com/5746044/>
>>>
>>
>>
>

[xur, 2012-03-15 23:03:41]

performance result b/w lto+fdo vs new lipo: (running on ikaria_westmere.
machines are not calipered.

Reference: lto00
(1): lipo00

          Benchmark            Base:Reference     (1)
--------------------------------------------------------
spec/2000/fp/C/177.mesa               0.02755     -0.75%
spec/2000/fp/C/179.art                 0.0216     -0.23%
spec/2000/fp/C/183.equake             0.03079     -4.41%
spec/2000/fp/C/188.ammp               0.01058     +2.97%
spec/2000/fp/F77/168.wupwise          0.01466     -0.66%
spec/2000/fp/F77/171.swim             0.01194     +1.15%
spec/2000/fp/F77/172.mgrid            0.01091     +6.10%
spec/2000/fp/F77/173.applu            0.01484     +0.21%
spec/2000/fp/F77/200.sixtrack         0.01017     -1.36%
spec/2000/fp/F77/301.apsi             0.01007     -0.67%
spec/2000/fp/F90/178.galgel          0.007485     -0.40%
spec/2000/fp/F90/187.facerec          0.01088     +2.36%
spec/2000/fp/F90/189.lucas            0.01487     +1.91%
spec/2000/fp/F90/191.fma3d           0.009243     +1.84%
spec/2000/int/C++/252.eon             0.03341     -1.46%
spec/2000/int/C/164.gzip              0.01249     +0.93%
spec/2000/int/C/175.vpr               0.01579     +2.13%
spec/2000/int/C/176.gcc               0.02891     -1.07%
spec/2000/int/C/181.mcf               0.01814     -0.23%
spec/2000/int/C/186.crafty            0.03316     +0.69%
spec/2000/int/C/197.parser             0.0118     +0.21%
spec/2000/int/C/253.perlbmk                --  [0.01839]
spec/2000/int/C/254.gap               0.02693     +2.12%
spec/2000/int/C/255.vortex            0.02947     -8.17%
spec/2000/int/C/256.bzip2             0.01418     -2.06%
spec/2000/int/C/300.twolf            0.008819     +0.37%

geometric mean                                    +0.03%



On Thu, Mar 15, 2012 at 1:24 PM, Rong Xu <xur@google.com> wrote:

>
>
> On Thu, Mar 15, 2012 at 11:16 AM, Rong Xu <xur@google.com> wrote:
>
>> I sent out patchset 2 to
>> http://codereview.appspot.com/5746044/
>>
>> It incorporates David's feedbacks and suggestions. Some of the LTO
>> bug fixes are still included to make the patch complete.
>>
>> Tested with bootstrap.
>>
>> Test this new lipo (-fripa=streaming) with SPEC2000, SPEC2006. This time
>> I included all Fortran programs.
>> 178.galgel and 187.facerec failed at profile-gen compilation.
>>
>
> 178.galgel and 187.facerec passed with manually rerun. It's either a
> perflab setup issue or glitch.
>
>
>>  447.dealII failed due to source problem.
>> 459.GemsFDTD failed in profile-gen compilation.
>> 481.wrf failed at run time of optimize binary (I will take a look at
>> this).
>>
>
> 459.GemsFDTD fails to build with -O2
> 481.wrf has run-time error with -O2.
> So they are not related to this patch.
>
>
>>
>> Perflab testing (existing lipo and this new lipo) is undergoing.
>>
>> -Rong
>>
>>
>> On Wed, Mar 14, 2012 at 4:12 PM, Rong Xu <xur@google.com> wrote:
>>
>>> replies are inlined.
>>>
>>> On Wed, Mar 14, 2012 at 2:50 PM, <davidxl@google.com> wrote:
>>>
>>>>
>>>>
http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**in.c<http://cod...
>>>> File lto-streamer-in.c (right):
>>>>
>>>> http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**
>>>>
in.c#newcode2698<http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c#newcode2698>
>>>> lto-streamer-in.c:2698: if (TREE_CODE (result) == VAR_DECL)
>>>> Is this change for LTO too?
>>>>
>>>> yes this affect LTO too. I did not guard it because I think this is a
>>> LTO bug. I will file a bug in upstream.
>>> I can guard the code with flag_ripa_stream (also need to change another
>>> place in lto.c).
>>> But I don't think anybody cares -- nobody is really use lto in
>>> google-4_6 branch.
>>>
>>>
>>>> http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**
>>>>
in.c#newcode2700<http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c#newcode2700>
>>>> lto-streamer-in.c:2700: /* This can be called in two places: (1) reading
>>>> decls, (2)
>>>> Fix indentation.
>>>>
>>>> Done.
>>>
>>>
>>>> http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**
>>>>
in.c#newcode2709<http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c#newcode2709>
>>>> lto-streamer-in.c:2709: {
>>>> Same here.
>>>>
>>>> Done.
>>>
>>>
>>>>
http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**out.c<http://co...
>>>> File lto-streamer-out.c (right):
>>>>
>>>> http://codereview.appspot.com/**5746044/diff/1/lto-streamer-**
>>>>
out.c#newcode2648<http://codereview.appspot.com/5746044/diff/1/lto-streamer-out.c#newcode2648>
>>>> lto-streamer-out.c:2648: unsigned char module_type = 0;
>>>> Not sure if using nested scope is the right style. Using a helper
>>>> function may be better.
>>>>
>>>> Will do.
>>>
>>>
>>>>
http://codereview.appspot.com/**5746044/diff/1/lto-symtab.c<http://codereview...
>>>> File lto-symtab.c (right):
>>>>
>>>> http://codereview.appspot.com/**5746044/diff/1/lto-symtab.c#**
>>>>
newcode153<http://codereview.appspot.com/5746044/diff/1/lto-symtab.c#newcode153>
>>>> lto-symtab.c:153: (2) have readonly static scope.
>>>> TREE_STATIC means the variable has definition -- so the comment is not
>>>> quite right.  The real reason is that aux module global vars will be
>>>> externalized, but the iniitialzer is still kept to enable const
>>>> propagation.
>>>>
>>>> you are probably right, I have that comment since the very beginning of
>>> the project.
>>> I'll double check and change it according.
>>>
>>>
>>>>
http://codereview.appspot.com/**5746044/<http://codereview.appspot.com/5746044/>
>>>>
>>>
>>>
>>
>

[davidxl, 2012-03-15 23:13:33]

equake is a single file program, right? Does it benefit from any whole
program analysis? It is interesting to find out the root cause of
vortex perf difference when the patch is submitted upstream.

David

On Thu, Mar 15, 2012 at 4:03 PM, Rong Xu <xur@google.com> wrote:
> performance result b/w lto+fdo vs new lipo: (running on ikaria_westmere.
> machines are not calipered.
>
> Reference: lto00
> (1): lipo00
>
>           Benchmark            Base:Reference     (1)
> --------------------------------------------------------
> spec/2000/fp/C/177.mesa               0.02755     -0.75%
> spec/2000/fp/C/179.art                 0.0216     -0.23%
> spec/2000/fp/C/183.equake             0.03079     -4.41%
> spec/2000/fp/C/188.ammp               0.01058     +2.97%
> spec/2000/fp/F77/168.wupwise          0.01466     -0.66%
> spec/2000/fp/F77/171.swim             0.01194     +1.15%
> spec/2000/fp/F77/172.mgrid            0.01091     +6.10%
> spec/2000/fp/F77/173.applu            0.01484     +0.21%
> spec/2000/fp/F77/200.sixtrack         0.01017     -1.36%
> spec/2000/fp/F77/301.apsi             0.01007     -0.67%
> spec/2000/fp/F90/178.galgel          0.007485     -0.40%
> spec/2000/fp/F90/187.facerec          0.01088     +2.36%
> spec/2000/fp/F90/189.lucas            0.01487     +1.91%
> spec/2000/fp/F90/191.fma3d           0.009243     +1.84%
> spec/2000/int/C++/252.eon             0.03341     -1.46%
> spec/2000/int/C/164.gzip              0.01249     +0.93%
> spec/2000/int/C/175.vpr               0.01579     +2.13%
> spec/2000/int/C/176.gcc               0.02891     -1.07%
> spec/2000/int/C/181.mcf               0.01814     -0.23%
> spec/2000/int/C/186.crafty            0.03316     +0.69%
> spec/2000/int/C/197.parser             0.0118     +0.21%
> spec/2000/int/C/253.perlbmk                --  [0.01839]
> spec/2000/int/C/254.gap               0.02693     +2.12%
> spec/2000/int/C/255.vortex            0.02947     -8.17%
> spec/2000/int/C/256.bzip2             0.01418     -2.06%
> spec/2000/int/C/300.twolf            0.008819     +0.37%
>
> geometric mean                                    +0.03%
>
>
>
> On Thu, Mar 15, 2012 at 1:24 PM, Rong Xu <xur@google.com> wrote:
>>
>>
>>
>> On Thu, Mar 15, 2012 at 11:16 AM, Rong Xu <xur@google.com> wrote:
>>>
>>> I sent out patchset 2 to
>>> http://codereview.appspot.com/5746044/
>>>
>>> It incorporates David's feedbacks and suggestions. Some of the LTO
>>> bug fixes are still included to make the patch complete.
>>>
>>> Tested with bootstrap.
>>>
>>> Test this new lipo (-fripa=streaming) with SPEC2000, SPEC2006. This time
>>> I included all Fortran programs.
>>> 178.galgel and 187.facerec failed at profile-gen compilation.
>>
>>
>> 178.galgel and 187.facerec passed with manually rerun. It's either a
>> perflab setup issue or glitch.
>>
>>>
>>> 447.dealII failed due to source problem.
>>> 459.GemsFDTD failed in profile-gen compilation.
>>> 481.wrf failed at run time of optimize binary (I will take a look at
>>> this).
>>
>>
>> 459.GemsFDTD fails to build with -O2
>> 481.wrf has run-time error with -O2.
>> So they are not related to this patch.
>>
>>>
>>>
>>> Perflab testing (existing lipo and this new lipo) is undergoing.
>>>
>>> -Rong
>>>
>>>
>>> On Wed, Mar 14, 2012 at 4:12 PM, Rong Xu <xur@google.com> wrote:
>>>>
>>>> replies are inlined.
>>>>
>>>> On Wed, Mar 14, 2012 at 2:50 PM, <davidxl@google.com> wrote:
>>>>>
>>>>>
>>>>> http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c
>>>>> File lto-streamer-in.c (right):
>>>>>
>>>>>
>>>>> http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c#newcode2698
>>>>> lto-streamer-in.c:2698: if (TREE_CODE (result) == VAR_DECL)
>>>>> Is this change for LTO too?
>>>>>
>>>> yes this affect LTO too. I did not guard it because I think this is a
>>>> LTO bug. I will file a bug in upstream.
>>>> I can guard the code with flag_ripa_stream (also need to change another
>>>> place in lto.c).
>>>> But I don't think anybody cares -- nobody is really use lto in
>>>> google-4_6 branch.
>>>>
>>>>>
>>>>>
>>>>> http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c#newcode2700
>>>>> lto-streamer-in.c:2700: /* This can be called in two places: (1)
>>>>> reading
>>>>> decls, (2)
>>>>> Fix indentation.
>>>>>
>>>> Done.
>>>>
>>>>>
>>>>>
>>>>> http://codereview.appspot.com/5746044/diff/1/lto-streamer-in.c#newcode2709
>>>>> lto-streamer-in.c:2709: {
>>>>> Same here.
>>>>>
>>>> Done.
>>>>
>>>>>
>>>>> http://codereview.appspot.com/5746044/diff/1/lto-streamer-out.c
>>>>> File lto-streamer-out.c (right):
>>>>>
>>>>>
>>>>>
http://codereview.appspot.com/5746044/diff/1/lto-streamer-out.c#newcode2648
>>>>> lto-streamer-out.c:2648: unsigned char module_type = 0;
>>>>> Not sure if using nested scope is the right style. Using a helper
>>>>> function may be better.
>>>>>
>>>> Will do.
>>>>
>>>>>
>>>>> http://codereview.appspot.com/5746044/diff/1/lto-symtab.c
>>>>> File lto-symtab.c (right):
>>>>>
>>>>> http://codereview.appspot.com/5746044/diff/1/lto-symtab.c#newcode153
>>>>> lto-symtab.c:153: (2) have readonly static scope.
>>>>> TREE_STATIC means the variable has definition -- so the comment is not
>>>>> quite right.  The real reason is that aux module global vars will be
>>>>> externalized, but the iniitialzer is still kept to enable const
>>>>> propagation.
>>>>>
>>>> you are probably right, I have that comment since the very beginning of
>>>> the project.
>>>> I'll double check and change it according.
>>>>
>>>>>
>>>>> http://codereview.appspot.com/5746044/
>>>>
>>>>
>>>
>>
>

[davidxl, 2012-03-16 16:35:54]

ok for google-46 after the minor changes below. Make sure default lipo testing
is well covered too.

David

http://codereview.appspot.com/5746044/diff/11001/cgraph.c
File cgraph.c (right):

http://codereview.appspot.com/5746044/diff/11001/cgraph.c#newcode2887
cgraph.c:2887: && !(L_IPO_STREAM_IN_LTO_P && cgraph_is_auxiliary(node->decl)))
Missing space

http://codereview.appspot.com/5746044/diff/11001/coverage.c
File coverage.c (right):

http://codereview.appspot.com/5746044/diff/11001/coverage.c#newcode396
coverage.c:396: warning (0, "error in opening %s, exiting",
0 -> UNKNOWN_LOCATION

http://codereview.appspot.com/5746044/diff/11001/coverage.c#newcode724
coverage.c:724: module_infos, num_in_fnames);
Identation

http://codereview.appspot.com/5746044/diff/11001/ipa.c
File ipa.c (right):

http://codereview.appspot.com/5746044/diff/11001/ipa.c#newcode678
ipa.c:678: Return true will make this symbol externally visible.
Returning

http://codereview.appspot.com/5746044/diff/11001/ipa.c#newcode679
ipa.c:679: Refurn false will continue current visibiliity logic.  */
Returning

[stevenb.gcc, 2012-03-16 17:32:40]

One spelling nit.

May also want to explain the 'ripa' term more clearly somewhere.

http://codereview.appspot.com/5746044/diff/11001/ipa.c
File ipa.c (right):

http://codereview.appspot.com/5746044/diff/11001/ipa.c#newcode811
ipa.c:811: (2) the VNODE is neede, and
s/neede/needed/