* Disable early inlining while compiling for coverage (issue5173042)
@ 2011-10-01 1:12 Sharad Singhai
2011-10-01 4:15 ` Xinliang David Li
0 siblings, 1 reply; 10+ messages in thread
From: Sharad Singhai @ 2011-10-01 1:12 UTC (permalink / raw)
To: reply, davidxl, gcc-patches
This patch disables early inlining when --coverage option is
specified. This improves coverage data in presence of other
optimizations, specially with -O2 where early inlining changes the
control flow graph sufficiently enough to generate seemingly very odd
source coverage.
Bootstrapped okay and regression tests passed.
Okay for google/gcc-4_6?
2011-09-30 Sharad Singhai <singhai@google.com>
* gcc.c (cc1_options): Added -fno-early-inlining for coverage.
Index: gcc.c
===================================================================
--- gcc.c (revision 179402)
+++ gcc.c (working copy)
@@ -776,7 +776,7 @@
%{!fsyntax-only:%{S:%W{o*}%{!o*:-o %b.s}}}\
%{fsyntax-only:-o %j} %{-param*}\
%{fmudflap|fmudflapth:-fno-builtin -fno-merge-constants}\
- %{coverage:-fprofile-arcs -ftest-coverage}";
+ %{coverage:-fprofile-arcs -ftest-coverage -fno-early-inlining}";
/* If an assembler wrapper is used to invoke post-assembly tools
like MAO, --save-temps need to be passed to save the output of
--
This patch is available for review at http://codereview.appspot.com/5173042
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Disable early inlining while compiling for coverage (issue5173042)
2011-10-01 1:12 Disable early inlining while compiling for coverage (issue5173042) Sharad Singhai
@ 2011-10-01 4:15 ` Xinliang David Li
2011-10-01 5:37 ` Sharad Singhai (शरद सिंघई)
2011-10-01 12:17 ` Jan Hubicka
0 siblings, 2 replies; 10+ messages in thread
From: Xinliang David Li @ 2011-10-01 4:15 UTC (permalink / raw)
To: Sharad Singhai; +Cc: reply, gcc-patches
Yes, this will improve test coverage option's usability, but please
provide the example to explain the issues.
David
On Fri, Sep 30, 2011 at 6:12 PM, Sharad Singhai <singhai@google.com> wrote:
> This patch disables early inlining when --coverage option is
> specified. This improves coverage data in presence of other
> optimizations, specially with -O2 where early inlining changes the
> control flow graph sufficiently enough to generate seemingly very odd
> source coverage.
>
> Bootstrapped okay and regression tests passed.
>
> Okay for google/gcc-4_6?
>
> 2011-09-30 Sharad Singhai <singhai@google.com>
>
> * gcc.c (cc1_options): Added -fno-early-inlining for coverage.
>
> Index: gcc.c
> ===================================================================
> --- gcc.c (revision 179402)
> +++ gcc.c (working copy)
> @@ -776,7 +776,7 @@
> %{!fsyntax-only:%{S:%W{o*}%{!o*:-o %b.s}}}\
> %{fsyntax-only:-o %j} %{-param*}\
> %{fmudflap|fmudflapth:-fno-builtin -fno-merge-constants}\
> - %{coverage:-fprofile-arcs -ftest-coverage}";
> + %{coverage:-fprofile-arcs -ftest-coverage -fno-early-inlining}";
>
> /* If an assembler wrapper is used to invoke post-assembly tools
> like MAO, --save-temps need to be passed to save the output of
>
> --
> This patch is available for review at http://codereview.appspot.com/5173042
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Disable early inlining while compiling for coverage (issue5173042)
2011-10-01 4:15 ` Xinliang David Li
@ 2011-10-01 5:37 ` Sharad Singhai (शरद सिंघई)
2011-10-01 12:17 ` Jan Hubicka
1 sibling, 0 replies; 10+ messages in thread
From: Sharad Singhai (शरद सिंघई) @ 2011-10-01 5:37 UTC (permalink / raw)
To: Xinliang David Li; +Cc: reply, gcc-patches
Here is an example. In the attached file, foo.c contains only two
functions, 'sum' and 'main'. The function 'sum' gets inlined into
'main' (with -O2).
gcc --coverage -O2 foo.c
./a.out
gcov -b foo.c
Now the coverage data for the 'if' condition in 'sum' looks like this:
(in attached file foo.c.gcov)
8: 8: if (v[i]) total += 1;
branch 0 never executed
branch 1 never executed
branch 2 taken 75% (fallthrough)
branch 3 taken 25%
Thus a simple conditional looks like a four-way branch. It is due to
early inlining where a couple of basic blocks get eliminated but the
branch coverage still gets attributed to the conditional. Similarly
the coverage data for the loop in 'sum' looks like this
9: 7: for (i = 0; i < N; ++i) {
branch 0 never executed
branch 1 never executed
branch 2 taken 89%
branch 3 taken 11% (fallthrough)
After disabling early inlining, the coverage data looks saner.
Of course, in general, the coverage data cannot be accurate in
presence of optimizations. However, this improves the situation
somewhat and improves usability when compiling without optimization is
not feasible.
Sharad
On Fri, Sep 30, 2011 at 9:15 PM, Xinliang David Li <davidxl@google.com> wrote:
>
> Yes, this will improve test coverage option's usability, but please
> provide the example to explain the issues.
>
> David
>
> On Fri, Sep 30, 2011 at 6:12 PM, Sharad Singhai <singhai@google.com> wrote:
> > This patch disables early inlining when --coverage option is
> > specified. This improves coverage data in presence of other
> > optimizations, specially with -O2 where early inlining changes the
> > control flow graph sufficiently enough to generate seemingly very odd
> > source coverage.
> >
> > Bootstrapped okay and regression tests passed.
> >
> > Okay for google/gcc-4_6?
> >
> > 2011-09-30 Sharad Singhai <singhai@google.com>
> >
> > * gcc.c (cc1_options): Added -fno-early-inlining for coverage.
> >
> > Index: gcc.c
> > ===================================================================
> > --- gcc.c (revision 179402)
> > +++ gcc.c (working copy)
> > @@ -776,7 +776,7 @@
> > %{!fsyntax-only:%{S:%W{o*}%{!o*:-o %b.s}}}\
> > %{fsyntax-only:-o %j} %{-param*}\
> > %{fmudflap|fmudflapth:-fno-builtin -fno-merge-constants}\
> > - %{coverage:-fprofile-arcs -ftest-coverage}";
> > + %{coverage:-fprofile-arcs -ftest-coverage -fno-early-inlining}";
> >
> > /* If an assembler wrapper is used to invoke post-assembly tools
> > like MAO, --save-temps need to be passed to save the output of
> >
> > --
> > This patch is available for review at http://codereview.appspot.com/5173042
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Disable early inlining while compiling for coverage (issue5173042)
2011-10-01 4:15 ` Xinliang David Li
2011-10-01 5:37 ` Sharad Singhai (शरद सिंघई)
@ 2011-10-01 12:17 ` Jan Hubicka
2011-10-01 17:19 ` Xinliang David Li
1 sibling, 1 reply; 10+ messages in thread
From: Jan Hubicka @ 2011-10-01 12:17 UTC (permalink / raw)
To: Xinliang David Li; +Cc: Sharad Singhai, reply, gcc-patches
> Yes, this will improve test coverage option's usability, but please
> provide the example to explain the issues.
>
> David
>
> On Fri, Sep 30, 2011 at 6:12 PM, Sharad Singhai <singhai@google.com> wrote:
> > This patch disables early inlining when --coverage option is
> > specified. This improves coverage data in presence of other
> > optimizations, specially with -O2 where early inlining changes the
> > control flow graph sufficiently enough to generate seemingly very odd
> > source coverage.
BTW early inlining was introduced to make coverage possible on some C++
sources, like tramp3d ;) However the problem here is that since we moved
coverage to SSA, we do it too late. My longer term plan is to separate
coverage and profile feedback passes (so they can't be done both together) and
instrument early for coverage & ensure that everything before coverage is done
is save WRT line info. Inlining alone does not mess up the line info, but most
optimizations we have in early optimization queue do.
We discussed it back when Richi implemented SSA profiling but we didn't do that
basically due to lack of testcases. Would be possible to take one you have
and fill in some PRs? Those are regressions WRT pre-SSA profiling releases (I think 4.5?)
Honza
> >
> > Bootstrapped okay and regression tests passed.
> >
> > Okay for google/gcc-4_6?
> >
> > 2011-09-30  Sharad Singhai  <singhai@google.com>
> >
> > Â Â Â Â * gcc.c (cc1_options): Added -fno-early-inlining for coverage.
> >
> > Index: gcc.c
> > ===================================================================
> > --- gcc.c    (revision 179402)
> > +++ gcc.c    (working copy)
> > @@ -776,7 +776,7 @@
> > Â %{!fsyntax-only:%{S:%W{o*}%{!o*:-o %b.s}}}\
> > Â %{fsyntax-only:-o %j} %{-param*}\
> > Â %{fmudflap|fmudflapth:-fno-builtin -fno-merge-constants}\
> > - %{coverage:-fprofile-arcs -ftest-coverage}";
> > + %{coverage:-fprofile-arcs -ftest-coverage -fno-early-inlining}";
> >
> > Â /* If an assembler wrapper is used to invoke post-assembly tools
> > Â Â like MAO, --save-temps need to be passed to save the output of
> >
> > --
> > This patch is available for review at http://codereview.appspot.com/5173042
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Disable early inlining while compiling for coverage (issue5173042)
2011-10-01 12:17 ` Jan Hubicka
@ 2011-10-01 17:19 ` Xinliang David Li
2011-10-01 19:49 ` Sharad Singhai (शरद सिंघई)
2011-10-02 10:04 ` Jan Hubicka
0 siblings, 2 replies; 10+ messages in thread
From: Xinliang David Li @ 2011-10-01 17:19 UTC (permalink / raw)
To: Jan Hubicka; +Cc: Sharad Singhai, reply, gcc-patches
On Sat, Oct 1, 2011 at 5:17 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Yes, this will improve test coverage option's usability, but please
>> provide the example to explain the issues.
>>
>> David
>>
>> On Fri, Sep 30, 2011 at 6:12 PM, Sharad Singhai <singhai@google.com> wrote:
>> > This patch disables early inlining when --coverage option is
>> > specified. This improves coverage data in presence of other
>> > optimizations, specially with -O2 where early inlining changes the
>> > control flow graph sufficiently enough to generate seemingly very odd
>> > source coverage.
>
> BTW early inlining was introduced to make coverage possible on some C++
> sources, like tramp3d ;)
Early inline can be important for FDO performance reasons -- as inline
instances get 'context' sensitive profile data.
>However the problem here is that since we moved
> coverage to SSA, we do it too late. My longer term plan is to separate
> coverage and profile feedback passes (so they can't be done both together) and
> instrument early for coverage & ensure that everything before coverage is done
> is save WRT line info.
Yes, coverage and FDO are two different animals having different
requirements -- they happen to share the same instrumentation
mechanism.
> Inlining alone does not mess up the line info, but most
> optimizations we have in early optimization queue do.
Inlining can do some damage too but to a less extent. For instance,
the exit block of the callee instance merged with caller's bb causing
confusion.
>
> We discussed it back when Richi implemented SSA profiling but we didn't do that
> basically due to lack of testcases. Would be possible to take one you have
> and fill in some PRs? Those are regressions WRT pre-SSA profiling releases (I think 4.5?)
Yes.
Sharad, I did not see the test case attached? Please file a bug about
this. In the meantime, you can checkin the workaround to google
banches.
thanks,
David
>
> Honza
>> >
>> > Bootstrapped okay and regression tests passed.
>> >
>> > Okay for google/gcc-4_6?
>> >
>> > 2011-09-30 Sharad Singhai <singhai@google.com>
>> >
>> > * gcc.c (cc1_options): Added -fno-early-inlining for coverage.
>> >
>> > Index: gcc.c
>> > ===================================================================
>> > --- gcc.c (revision 179402)
>> > +++ gcc.c (working copy)
>> > @@ -776,7 +776,7 @@
>> > %{!fsyntax-only:%{S:%W{o*}%{!o*:-o %b.s}}}\
>> > %{fsyntax-only:-o %j} %{-param*}\
>> > %{fmudflap|fmudflapth:-fno-builtin -fno-merge-constants}\
>> > - %{coverage:-fprofile-arcs -ftest-coverage}";
>> > + %{coverage:-fprofile-arcs -ftest-coverage -fno-early-inlining}";
>> >
>> > /* If an assembler wrapper is used to invoke post-assembly tools
>> > like MAO, --save-temps need to be passed to save the output of
>> >
>> > --
>> > This patch is available for review at http://codereview.appspot.com/5173042
>> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Disable early inlining while compiling for coverage (issue5173042)
2011-10-01 17:19 ` Xinliang David Li
@ 2011-10-01 19:49 ` Sharad Singhai (शरद सिंघई)
2011-10-02 10:04 ` Jan Hubicka
1 sibling, 0 replies; 10+ messages in thread
From: Sharad Singhai (शरद सिंघई) @ 2011-10-01 19:49 UTC (permalink / raw)
To: Xinliang David Li; +Cc: Jan Hubicka, reply, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 3400 bytes --]
I have updated PR/45890 with a test case.
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45890
In any case, I am attaching a test case here as well.
Sharad
On Sat, Oct 1, 2011 at 10:19 AM, Xinliang David Li <davidxl@google.com> wrote:
>
> On Sat, Oct 1, 2011 at 5:17 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> >> Yes, this will improve test coverage option's usability, but please
> >> provide the example to explain the issues.
> >>
> >> David
> >>
> >> On Fri, Sep 30, 2011 at 6:12 PM, Sharad Singhai <singhai@google.com> wrote:
> >> > This patch disables early inlining when --coverage option is
> >> > specified. This improves coverage data in presence of other
> >> > optimizations, specially with -O2 where early inlining changes the
> >> > control flow graph sufficiently enough to generate seemingly very odd
> >> > source coverage.
> >
> > BTW early inlining was introduced to make coverage possible on some C++
> > sources, like tramp3d ;)
>
> Early inline can be important for FDO performance reasons -- as inline
> instances get 'context' sensitive profile data.
>
> >However the problem here is that since we moved
> > coverage to SSA, we do it too late. My longer term plan is to separate
> > coverage and profile feedback passes (so they can't be done both together) and
> > instrument early for coverage & ensure that everything before coverage is done
> > is save WRT line info.
>
> Yes, coverage and FDO are two different animals having different
> requirements -- they happen to share the same instrumentation
> mechanism.
>
> > Inlining alone does not mess up the line info, but most
> > optimizations we have in early optimization queue do.
>
> Inlining can do some damage too but to a less extent. For instance,
> the exit block of the callee instance merged with caller's bb causing
> confusion.
>
> >
> > We discussed it back when Richi implemented SSA profiling but we didn't do that
> > basically due to lack of testcases. Would be possible to take one you have
> > and fill in some PRs? Those are regressions WRT pre-SSA profiling releases (I think 4.5?)
>
> Yes.
>
> Sharad, I did not see the test case attached? Please file a bug about
> this. In the meantime, you can checkin the workaround to google
> banches.
>
> thanks,
>
> David
>
> >
> > Honza
> >> >
> >> > Bootstrapped okay and regression tests passed.
> >> >
> >> > Okay for google/gcc-4_6?
> >> >
> >> > 2011-09-30 Sharad Singhai <singhai@google.com>
> >> >
> >> > * gcc.c (cc1_options): Added -fno-early-inlining for coverage.
> >> >
> >> > Index: gcc.c
> >> > ===================================================================
> >> > --- gcc.c (revision 179402)
> >> > +++ gcc.c (working copy)
> >> > @@ -776,7 +776,7 @@
> >> > %{!fsyntax-only:%{S:%W{o*}%{!o*:-o %b.s}}}\
> >> > %{fsyntax-only:-o %j} %{-param*}\
> >> > %{fmudflap|fmudflapth:-fno-builtin -fno-merge-constants}\
> >> > - %{coverage:-fprofile-arcs -ftest-coverage}";
> >> > + %{coverage:-fprofile-arcs -ftest-coverage -fno-early-inlining}";
> >> >
> >> > /* If an assembler wrapper is used to invoke post-assembly tools
> >> > like MAO, --save-temps need to be passed to save the output of
> >> >
> >> > --
> >> > This patch is available for review at http://codereview.appspot.com/5173042
> >> >
> >
[-- Attachment #2: gcov.tar.gz --]
[-- Type: application/x-gzip, Size: 791 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Disable early inlining while compiling for coverage (issue5173042)
2011-10-01 17:19 ` Xinliang David Li
2011-10-01 19:49 ` Sharad Singhai (शरद सिंघई)
@ 2011-10-02 10:04 ` Jan Hubicka
2011-10-02 10:09 ` Jan Hubicka
1 sibling, 1 reply; 10+ messages in thread
From: Jan Hubicka @ 2011-10-02 10:04 UTC (permalink / raw)
To: Xinliang David Li; +Cc: Jan Hubicka, Sharad Singhai, reply, gcc-patches
>
> Early inline can be important for FDO performance reasons -- as inline
> instances get 'context' sensitive profile data.
Yep, early inlining combine well with FDO.
> > Inlining alone does not mess up the line info, but most
> > optimizations we have in early optimization queue do.
>
> Inlining can do some damage too but to a less extent. For instance,
> the exit block of the callee instance merged with caller's bb causing
> confusion.
This is still the cfgcleanup run after inlining, right?
But anyway, doing coverage instrumentation before inlining (just after going to SSA)
seems fine to me.
We used to have cfgcleanup mode that is safe WRT line number info, not sure if it still
in there.
>
> >
> > We discussed it back when Richi implemented SSA profiling but we didn't do that
> > basically due to lack of testcases. Â Would be possible to take one you have
> > and fill in some PRs? Those are regressions WRT pre-SSA profiling releases (I think 4.5?)
>
> Yes.
>
> Sharad, I did not see the test case attached? Please file a bug about
> this. In the meantime, you can checkin the workaround to google
> banches.
I believe Richi opent PR back when he introduced the SSA profiling, but I don;t seem
to be able to find it now.
Honza
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Disable early inlining while compiling for coverage (issue5173042)
2011-10-02 10:04 ` Jan Hubicka
@ 2011-10-02 10:09 ` Jan Hubicka
2011-11-07 21:40 ` Sharad Singhai
0 siblings, 1 reply; 10+ messages in thread
From: Jan Hubicka @ 2011-10-02 10:09 UTC (permalink / raw)
To: Jan Hubicka; +Cc: Xinliang David Li, Sharad Singhai, reply, gcc-patches
>
> I believe Richi opent PR back when he introduced the SSA profiling, but I don;t seem
> to be able to find it now.
Ha, you found it. It is PR gcov-profile/45890.
Given outcome of this dicussion I think it would make most sense to make coverage just after into-SSA
(and perhaps arrange cfgcleanups run before to be sensitive WRT line number infos).
Path is welcome, otherwise I will try to put this on my TODO for 4.7
Thanks!
Honza
>
> Honza
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Disable early inlining while compiling for coverage (issue5173042)
2011-10-02 10:09 ` Jan Hubicka
@ 2011-11-07 21:40 ` Sharad Singhai
2011-11-07 22:16 ` Xinliang David Li
0 siblings, 1 reply; 10+ messages in thread
From: Sharad Singhai @ 2011-11-07 21:40 UTC (permalink / raw)
To: Xinliang David Li, Jan Hubicka; +Cc: reply, gcc-patches
Honza,
Sorry, I forgot about this. Could you please put this on your TODO list?
David,
While a proper fix is pending for the trunk, we need this interim fix
internally. Okay for google/main?
Thanks,
Sharad
On Sun, Oct 2, 2011 at 3:08 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>
>> I believe Richi opent PR back when he introduced the SSA profiling, but I don;t seem
>> to be able to find it now.
> Ha, you found it. It is PR gcov-profile/45890.
>
> Given outcome of this dicussion I think it would make most sense to make coverage just after into-SSA
> (and perhaps arrange cfgcleanups run before to be sensitive WRT line number infos).
> Path is welcome, otherwise I will try to put this on my TODO for 4.7
>
> Thanks!
> Honza
>>
>> Honza
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Disable early inlining while compiling for coverage (issue5173042)
2011-11-07 21:40 ` Sharad Singhai
@ 2011-11-07 22:16 ` Xinliang David Li
0 siblings, 0 replies; 10+ messages in thread
From: Xinliang David Li @ 2011-11-07 22:16 UTC (permalink / raw)
To: Sharad Singhai; +Cc: Jan Hubicka, reply, gcc-patches
On Mon, Nov 7, 2011 at 1:30 PM, Sharad Singhai <singhai@google.com> wrote:
> Honza,
> Sorry, I forgot about this. Could you please put this on your TODO list?
>
> David,
> While a proper fix is pending for the trunk, we need this interim fix
> internally. Okay for google/main?
ok.
thanks,
David
>
> Thanks,
> Sharad
>
> On Sun, Oct 2, 2011 at 3:08 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>>
>>> I believe Richi opent PR back when he introduced the SSA profiling, but I don;t seem
>>> to be able to find it now.
>> Ha, you found it. It is PR gcov-profile/45890.
>>
>> Given outcome of this dicussion I think it would make most sense to make coverage just after into-SSA
>> (and perhaps arrange cfgcleanups run before to be sensitive WRT line number infos).
>> Path is welcome, otherwise I will try to put this on my TODO for 4.7
>>
>> Thanks!
>> Honza
>>>
>>> Honza
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-11-07 22:06 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-10-01 1:12 Disable early inlining while compiling for coverage (issue5173042) Sharad Singhai
2011-10-01 4:15 ` Xinliang David Li
2011-10-01 5:37 ` Sharad Singhai (शरद सिंघई)
2011-10-01 12:17 ` Jan Hubicka
2011-10-01 17:19 ` Xinliang David Li
2011-10-01 19:49 ` Sharad Singhai (शरद सिंघई)
2011-10-02 10:04 ` Jan Hubicka
2011-10-02 10:09 ` Jan Hubicka
2011-11-07 21:40 ` Sharad Singhai
2011-11-07 22:16 ` Xinliang David Li
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).