Re: [PATCH, 5.x/6.x/7.x] Be more conservative in early inliner if FDO is enabled

["Yuan, Pengfei" <ypf@pku.edu.cn>]

> > Here are the results:
> > 
> > Param  Size (GCC5)        Time (GCC5)  Time (GCC7)
> > 0      44686265 (-8.26%)  58.772s      66.332s
> > 1      45692793 (-6.19%)  40.684s      39.220s
> > 2      45556185 (-6.47%)  35.292s      34.328s
> > 3      46251049 (-5.05%)  28.820s      27.136s
> > 4      47028873 (-3.45%)  24.616s      22.200s
> > 5      47495641 (-2.49%)  20.160s      17.800s
> > 6      47520153 (-2.44%)  16.444s      15.656s
> > 14     48708873           5.620s       5.556s
> 
> Thanks for data! I meant to run the benchmark myself, but had little time to do
> it over past week becuase of traveling and was also wondering what to do given
> that spec is rather poor benchmark in this area. Tramp3d is biassed but we are
> in stage1 and can fine tune latter. I am debugging the libxul crashes in FDO
> binary now, so we can re-run talos.

It seems to be memory corruption. The content of a pointer becomes 0xe5e5...e5.
I have tried Firefox 48.0.2, 49b10 and mozilla-central with GCC 6. Same error.

> > Param: value of PARAM_EARLY_INLINING_INSNS
> > Size:  code size (.text) of optimized libxul.so
> > Time:  execution time of instrumented tramp3d (-n 25)
> > 
> > To balance between size reduction of optimized binary and speed penalty
> > of instrumented binary, I set param=6 as baseline and compare:
> > 
> > Param  Size score  Time score  Total
> > 0      3.39        -3.57       -0.18
> > 1      2.54        -2.47        0.07
> > 2      2.65        -2.15        0.50
> > 3      2.07        -1.75        0.32
> > 4      1.41        -1.50       -0.09
> > 5      1.02        -1.23       -0.21
> > 6      1.00        -1.00        0.00
> > 14     0.00        -0.34       -0.34
> > 
> > Therefore, I think param=2 is the best choice.
> > 
> > Is the attached patch OK?
> 
> Setting param to 2 looks fine
> 
> > gcc/ChangeLog
> > 	* opts.c (finish_options): Adjust PARAM_EARLY_INLINING_INSNS
> > 	when FDO is enabled.
> > 
> > 
> > diff --git a/gcc/opts.c b/gcc/opts.c
> > index 39c190d..b59c700 100644
> > --- a/gcc/opts.c
> > +++ b/gcc/opts.c
> > @@ -826,8 +826,14 @@ finish_options (struct gcc_options *opts, struct gcc_options *opts_set,
> >        maybe_set_param_value (PARAM_STACK_FRAME_GROWTH, 40,
> >                              opts->x_param_values, opts_set->x_param_values);
> >      }
> > 
> > +  /* Adjust PARAM_EARLY_INLINING_INSNS when FDO is enabled.  */
> > +  if ((opts->x_profile_arc_flag && !opts->x_flag_test_coverage)
> > +      || (opts->x_flag_branch_probabilities && !opts->x_flag_auto_profile))
> > +    maybe_set_param_value (PARAM_EARLY_INLINING_INSNS, 2,
> > +                          opts->x_param_values, opts_set->x_param_values);
> > +
> 
> I would actually preffer to have PARAM_EARLY_ININING_INSNS_FEEDBACK.  We
> already have TRACER_DYNAMIC_COVERAGE_FEEDBACK and other params.  The reason is
> that profile is not a global property of program. It may or may not be
> available for given function, while params are global.

Whether profile is available for specific functions is not important here because
profile is loaded after pass_early_inline. Therefore I think setting the param
globally is fine.

> Even at compile time profile may be selectively missing for example for
> COMDATs that did not win in the linking process.
> 
> There is also need to update the documentation.
> Thanks for the work!
> Honza

Here is the new patch.

Regards,
Yuan, Pengfei


gcc/ChangeLog
	* doc/invoke.texi (early-inlining-insns): Value is adjusted
	when FDO is enabled.
	* opts.c (finish_options): Adjust PARAM_EARLY_INLINING_INSNS
	when FDO is enabled.


diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 1ca4dcc..880a28c 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -10272,9 +10272,11 @@ The default value is 10.
 
 @item early-inlining-insns
 Specify growth that the early inliner can make.  In effect it increases
 the amount of inlining for code having a large abstraction penalty.
-The default value is 14.
+The default value is 14.  When feedback-directed optimizations is enabled (by
+@option{-fprofile-generate} or @option{-fprofile-use}), the value is adjusted
+to 2.
 
 @item max-early-inliner-iterations
 Limit of iterations of the early inliner.  This basically bounds
 the number of nested indirect calls the early inliner can resolve.
diff --git a/gcc/opts.c b/gcc/opts.c
index 39c190d..b59c700 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -826,8 +826,14 @@ finish_options (struct gcc_options *opts, struct gcc_options *opts_set,
       maybe_set_param_value (PARAM_STACK_FRAME_GROWTH, 40,
 			     opts->x_param_values, opts_set->x_param_values);
     }
 
+  /* Adjust PARAM_EARLY_INLINING_INSNS when FDO is enabled.  */
+  if ((opts->x_profile_arc_flag && !opts->x_flag_test_coverage)
+      || (opts->x_flag_branch_probabilities && !opts->x_flag_auto_profile))
+    maybe_set_param_value (PARAM_EARLY_INLINING_INSNS, 2,
+			   opts->x_param_values, opts_set->x_param_values);
+
   if (opts->x_flag_lto)
     {
 #ifdef ENABLE_LTO
       opts->x_flag_generate_lto = 1;