Patchwork Disable tracer by default for profile use (issue4428074)

login
register
mail settings
Submitter Sharad Singhai
Date April 28, 2011, 6:51 p.m.
Message ID <20110428185125.936CE15C19E@nabu.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/93276/
State New
Headers show

Comments

Sharad Singhai - April 28, 2011, 6:51 p.m.
This patch disables -ftracer for profile use. Okay for google/main?

Thanks,
Sharad

2011-04-28  Sharad Singhai  <singhai@google.com>

	Google Ref 40087
	* opts.c (common_handle_option): Disable -ftracer for profile use.
	* doc/invoke.texi: Update doc that -ftracer is no longer enabled for FDO.


--
This patch is available for review at http://codereview.appspot.com/4428074
Diego Novillo - April 28, 2011, 6:53 p.m.
On Thu, Apr 28, 2011 at 14:51, Sharad Singhai <singhai@google.com> wrote:
> This patch disables -ftracer for profile use. Okay for google/main?

Could you elaborate on why doing this is beneficial?  Are you
proposing this for trunk as well?

> 2011-04-28  Sharad Singhai  <singhai@google.com>
>
>        Google Ref 40087
>        * opts.c (common_handle_option): Disable -ftracer for profile use.
>        * doc/invoke.texi: Update doc that -ftracer is no longer enabled for FDO.

OK.


Diego.
Richard Guenther - April 28, 2011, 11:23 p.m.
On Thu, Apr 28, 2011 at 8:53 PM, Diego Novillo <dnovillo@google.com> wrote:
> On Thu, Apr 28, 2011 at 14:51, Sharad Singhai <singhai@google.com> wrote:
>> This patch disables -ftracer for profile use. Okay for google/main?
>
> Could you elaborate on why doing this is beneficial?  Are you
> proposing this for trunk as well?

It indeed doesn't make sense to me at all.

Richard.

>> 2011-04-28  Sharad Singhai  <singhai@google.com>
>>
>>        Google Ref 40087
>>        * opts.c (common_handle_option): Disable -ftracer for profile use.
>>        * doc/invoke.texi: Update doc that -ftracer is no longer enabled for FDO.
>
> OK.
>
>
> Diego.
>
Xinliang David Li - April 28, 2011, 11:34 p.m.
Sharad can provide some some performance data -- we have seen up to 2%
degradation to with tracer turned on for one of google's most
important program. Perhaps Sharad can collect some SPEC numbers.

I agree a better approach should be to fix the problem in tracer
instead of turning it off in trunk.

David


On Thu, Apr 28, 2011 at 4:23 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Thu, Apr 28, 2011 at 8:53 PM, Diego Novillo <dnovillo@google.com> wrote:
>> On Thu, Apr 28, 2011 at 14:51, Sharad Singhai <singhai@google.com> wrote:
>>> This patch disables -ftracer for profile use. Okay for google/main?
>>
>> Could you elaborate on why doing this is beneficial?  Are you
>> proposing this for trunk as well?
>
> It indeed doesn't make sense to me at all.
>
> Richard.
>
>>> 2011-04-28  Sharad Singhai  <singhai@google.com>
>>>
>>>        Google Ref 40087
>>>        * opts.c (common_handle_option): Disable -ftracer for profile use.
>>>        * doc/invoke.texi: Update doc that -ftracer is no longer enabled for FDO.
>>
>> OK.
>>
>>
>> Diego.
>>
>
Richard Guenther - April 28, 2011, 11:38 p.m.
On Fri, Apr 29, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
> Sharad can provide some some performance data -- we have seen up to 2%
> degradation to with tracer turned on for one of google's most
> important program. Perhaps Sharad can collect some SPEC numbers.
>
> I agree a better approach should be to fix the problem in tracer
> instead of turning it off in trunk.

Esp. not turning it off for profile-use only where it should have the most
precise input.

Richard.

> David
>
>
> On Thu, Apr 28, 2011 at 4:23 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Thu, Apr 28, 2011 at 8:53 PM, Diego Novillo <dnovillo@google.com> wrote:
>>> On Thu, Apr 28, 2011 at 14:51, Sharad Singhai <singhai@google.com> wrote:
>>>> This patch disables -ftracer for profile use. Okay for google/main?
>>>
>>> Could you elaborate on why doing this is beneficial?  Are you
>>> proposing this for trunk as well?
>>
>> It indeed doesn't make sense to me at all.
>>
>> Richard.
>>
>>>> 2011-04-28  Sharad Singhai  <singhai@google.com>
>>>>
>>>>        Google Ref 40087
>>>>        * opts.c (common_handle_option): Disable -ftracer for profile use.
>>>>        * doc/invoke.texi: Update doc that -ftracer is no longer enabled for FDO.
>>>
>>> OK.
>>>
>>>
>>> Diego.
>>>
>>
>
Xinliang David Li - April 28, 2011, 11:43 p.m.
Tracer is not turned on by default for non-FDO case -- Sharad's change
simply turns it off by default for all cases.

David

On Thu, Apr 28, 2011 at 4:38 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Fri, Apr 29, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>> Sharad can provide some some performance data -- we have seen up to 2%
>> degradation to with tracer turned on for one of google's most
>> important program. Perhaps Sharad can collect some SPEC numbers.
>>
>> I agree a better approach should be to fix the problem in tracer
>> instead of turning it off in trunk.
>
> Esp. not turning it off for profile-use only where it should have the most
> precise input.
>
> Richard.
>
>> David
>>
>>
>> On Thu, Apr 28, 2011 at 4:23 PM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Thu, Apr 28, 2011 at 8:53 PM, Diego Novillo <dnovillo@google.com> wrote:
>>>> On Thu, Apr 28, 2011 at 14:51, Sharad Singhai <singhai@google.com> wrote:
>>>>> This patch disables -ftracer for profile use. Okay for google/main?
>>>>
>>>> Could you elaborate on why doing this is beneficial?  Are you
>>>> proposing this for trunk as well?
>>>
>>> It indeed doesn't make sense to me at all.
>>>
>>> Richard.
>>>
>>>>> 2011-04-28  Sharad Singhai  <singhai@google.com>
>>>>>
>>>>>        Google Ref 40087
>>>>>        * opts.c (common_handle_option): Disable -ftracer for profile use.
>>>>>        * doc/invoke.texi: Update doc that -ftracer is no longer enabled for FDO.
>>>>
>>>> OK.
>>>>
>>>>
>>>> Diego.
>>>>
>>>
>>
>
Richard Guenther - April 29, 2011, 8:26 a.m.
2011/4/29 Sharad Singhai (शरद सिंघई) <singhai@google.com>:
> On Thu, Apr 28, 2011 at 4:38 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>>
>> On Fri, Apr 29, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com>
>> wrote:
>> > Sharad can provide some some performance data -- we have seen up to 2%
>> > degradation to with tracer turned on for one of google's most
>> > important program. Perhaps Sharad can collect some SPEC numbers.
>> >
>> > I agree a better approach should be to fix the problem in tracer
>> > instead of turning it off in trunk.
>>
>> Esp. not turning it off for profile-use only where it should have the most
>> precise input.
>>
>
> Yes. As I explained, I am not proposing to turn -ftracer off in the trunk.

Ah, sorry for the confusion on my side.  If you can back up the change
with SPEC performance numbers it's ok for trunk as well.  But indeed
coverage of -ftracer will be zero then, and I wonder what it is useful
for if it doesn't even do good with FDO ...

Richard.

> After some tuning the pass should be improved.
> As of now, during the performance analysis of the internal benchmarks, I
> have seen up to 2% performance loss with -ftracer and hence turned it off
> during FDO.
> I would do SPEC performance numbers and send those out.
> Thanks,
> Sharad
>
>>
>> Richard.
>>
>> > David
>> >
>> >
>> > On Thu, Apr 28, 2011 at 4:23 PM, Richard Guenther
>> > <richard.guenther@gmail.com> wrote:
>> >> On Thu, Apr 28, 2011 at 8:53 PM, Diego Novillo <dnovillo@google.com>
>> >> wrote:
>> >>> On Thu, Apr 28, 2011 at 14:51, Sharad Singhai <singhai@google.com>
>> >>> wrote:
>> >>>> This patch disables -ftracer for profile use. Okay for google/main?
>> >>>
>> >>> Could you elaborate on why doing this is beneficial?  Are you
>> >>> proposing this for trunk as well?
>> >>
>> >> It indeed doesn't make sense to me at all.
>> >>
>> >> Richard.
>> >>
>> >>>> 2011-04-28  Sharad Singhai  <singhai@google.com>
>> >>>>
>> >>>>        Google Ref 40087
>> >>>>        * opts.c (common_handle_option): Disable -ftracer for profile
>> >>>> use.
>> >>>>        * doc/invoke.texi: Update doc that -ftracer is no longer
>> >>>> enabled for FDO.
>> >>>
>> >>> OK.
>> >>>
>> >>>
>> >>> Diego.
>> >>>
>> >>
>> >
>
>
Jan Hubicka - April 29, 2011, 11:24 p.m.
> 2011/4/29 Sharad Singhai (????????? ???????????????) <singhai@google.com>:
> > On Thu, Apr 28, 2011 at 4:38 PM, Richard Guenther
> > <richard.guenther@gmail.com> wrote:
> >>
> >> On Fri, Apr 29, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com>
> >> wrote:
> >> > Sharad can provide some some performance data -- we have seen up to 2%
> >> > degradation to with tracer turned on for one of google's most
> >> > important program. Perhaps Sharad can collect some SPEC numbers.
> >> >
> >> > I agree a better approach should be to fix the problem in tracer
> >> > instead of turning it off in trunk.
> >>
> >> Esp. not turning it off for profile-use only where it should have the most
> >> precise input.
> >>
> >
> > Yes. As I explained, I am not proposing to turn -ftracer off in the trunk.
> 
> Ah, sorry for the confusion on my side.  If you can back up the change
> with SPEC performance numbers it's ok for trunk as well.  But indeed
> coverage of -ftracer will be zero then, and I wonder what it is useful
> for if it doesn't even do good with FDO ...

Tracer definitely did measurable SPEC speedups at a time it was implemented.
So we are most likely running into some bug, like misupdated profile or
tracer confusing something more important.  If you provide me some testcase,
I can try to debug it.

Honza

Patch

Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 173048)
+++ doc/invoke.texi	(working copy)
@@ -7930,7 +7930,7 @@ 
 generally profitable only with profile feedback available.
 
 The following options are enabled: @code{-fbranch-probabilities}, @code{-fvpt},
-@code{-funroll-loops}, @code{-fpeel-loops}, @code{-ftracer}
+@code{-funroll-loops}, @code{-fpeel-loops}.
 
 By default, GCC emits an error message if the feedback profiles do not
 match the source code.  This error can be turned into a warning by using
Index: opts.c
===================================================================
--- opts.c	(revision 173048)
+++ opts.c	(working copy)
@@ -1531,8 +1531,6 @@ 
 	opts->x_flag_unroll_loops = value;
       if (!opts_set->x_flag_peel_loops)
 	opts->x_flag_peel_loops = value;
-      if (!opts_set->x_flag_tracer)
-	opts->x_flag_tracer = value;
       if (!opts_set->x_flag_value_profile_transformations)
 	opts->x_flag_value_profile_transformations = value;
       if (!opts_set->x_flag_inline_functions)