Message ID | 20221011124303.99673-1-jorgen.kvalsvik@woven-planet.global |
---|---|
State | New |
Headers | show |
Series | [1/2] gcov: test switch/break line counts | expand |
Hello, On Tue, 11 Oct 2022, Jørgen Kvalsvik via Gcc-patches wrote: > The coverage support will under some conditions decide to split edges to > accurately report coverage. By running the test suite with/without this > edge splitting a small diff shows up, addressed by this patch, which > should catch future regressions. > > Removing the edge splitting: > > $ diff --git a/gcc/profile.cc b/gcc/profile.cc > --- a/gcc/profile.cc > +++ b/gcc/profile.cc > @@ -1244,19 +1244,7 @@ branch_prob (bool thunk) > Don't do that when the locuses match, so > if (blah) goto something; > is not computed twice. */ > - if (last > - && gimple_has_location (last) > - && !RESERVED_LOCATION_P (e->goto_locus) > - && !single_succ_p (bb) > - && (LOCATION_FILE (e->goto_locus) > - != LOCATION_FILE (gimple_location (last)) > - || (LOCATION_LINE (e->goto_locus) > - != LOCATION_LINE (gimple_location (last))))) > - { > - basic_block new_bb = split_edge (e); > - edge ne = single_succ_edge (new_bb); > - ne->goto_locus = e->goto_locus; > - } > + Assuming this is correct (I really can't say) then the comment needs adjustments. It specifically talks about this very code you remove. Ciao, Michael.
On 11/10/2022 15:55, Michael Matz wrote: > Hello, > > On Tue, 11 Oct 2022, Jørgen Kvalsvik via Gcc-patches wrote: > >> The coverage support will under some conditions decide to split edges to >> accurately report coverage. By running the test suite with/without this >> edge splitting a small diff shows up, addressed by this patch, which >> should catch future regressions. >> >> Removing the edge splitting: >> >> $ diff --git a/gcc/profile.cc b/gcc/profile.cc >> --- a/gcc/profile.cc >> +++ b/gcc/profile.cc >> @@ -1244,19 +1244,7 @@ branch_prob (bool thunk) >> Don't do that when the locuses match, so >> if (blah) goto something; >> is not computed twice. */ >> - if (last >> - && gimple_has_location (last) >> - && !RESERVED_LOCATION_P (e->goto_locus) >> - && !single_succ_p (bb) >> - && (LOCATION_FILE (e->goto_locus) >> - != LOCATION_FILE (gimple_location (last)) >> - || (LOCATION_LINE (e->goto_locus) >> - != LOCATION_LINE (gimple_location (last))))) >> - { >> - basic_block new_bb = split_edge (e); >> - edge ne = single_succ_edge (new_bb); >> - ne->goto_locus = e->goto_locus; >> - } >> + > > Assuming this is correct (I really can't say) then the comment needs > adjustments. It specifically talks about this very code you remove. > > > Ciao, > Michael. Michael, I apologise for the confusion. The diff there is not a part of the change itself (note the indentation) but rather a way to reproduce, or at least understand, the type of change that would trigger the new test error. If it is too confusing I can re-write the commit message. Thanks, Jørgen
Hello, On Tue, 11 Oct 2022, Jørgen Kvalsvik wrote: > I apologise for the confusion. The diff there is not a part of the > change itself (note the indentation) but rather a way to reproduce, Ah! Thanks, that explains it, sorry for adding confusion on top :-) Ciao, Michael.
On Tue, Oct 11, 2022 at 2:43 PM Jørgen Kvalsvik <jorgen.kvalsvik@woven-planet.global> wrote: > > The coverage support will under some conditions decide to split edges to > accurately report coverage. By running the test suite with/without this > edge splitting a small diff shows up, addressed by this patch, which > should catch future regressions. > > Removing the edge splitting: > > $ diff --git a/gcc/profile.cc b/gcc/profile.cc > --- a/gcc/profile.cc > +++ b/gcc/profile.cc > @@ -1244,19 +1244,7 @@ branch_prob (bool thunk) > Don't do that when the locuses match, so > if (blah) goto something; > is not computed twice. */ > - if (last > - && gimple_has_location (last) > - && !RESERVED_LOCATION_P (e->goto_locus) > - && !single_succ_p (bb) > - && (LOCATION_FILE (e->goto_locus) > - != LOCATION_FILE (gimple_location (last)) > - || (LOCATION_LINE (e->goto_locus) > - != LOCATION_LINE (gimple_location (last))))) > - { > - basic_block new_bb = split_edge (e); > - edge ne = single_succ_edge (new_bb); > - ne->goto_locus = e->goto_locus; > - } > + > if ((e->flags & (EDGE_ABNORMAL | EDGE_ABNORMAL_CALL)) > && e->dest != EXIT_BLOCK_PTR_FOR_FN (cfun)) > need_exit_edge = 1; > > Assuming the .gcov files from make chec-gcc RUNTESTFLAGS=gcov.exp are > kept: > > $ diff -r no-split-edge with-split-edge | grep -C 2 -E "^[<>]\s\s" > diff -r sans-split-edge/gcc/gcov-4.c.gcov with-split-edge/gcc/gcov-4.c.gcov > 228c228 > < -: 224: break; > --- > > 1: 224: break; > 231c231 > < -: 227: break; > --- > > #####: 227: break; > 237c237 > < -: 233: break; > --- > > 2: 233: break; > > gcc/testsuite/ChangeLog: OK. Thanks, Richard. > > * g++.dg/gcov/gcov-1.C: Add line count check. > * gcc.misc-tests/gcov-4.c: Likewise. > --- > gcc/testsuite/g++.dg/gcov/gcov-1.C | 8 ++++---- > gcc/testsuite/gcc.misc-tests/gcov-4.c | 4 ++-- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C b/gcc/testsuite/g++.dg/gcov/gcov-1.C > index 9018b9a3a73..ee383b480a8 100644 > --- a/gcc/testsuite/g++.dg/gcov/gcov-1.C > +++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C > @@ -257,20 +257,20 @@ test_switch (int i, int j) > switch (i) /* count(5) */ > /* branch(end) */ > { > - case 1: > + case 1: /* count(1) */ > result = do_something (2); /* count(1) */ > - break; > + break; /* count(1) */ > case 2: > result = do_something (1024); > break; > - case 3: > + case 3: /* count(3) */ > case 4: > /* branch(67) */ > if (j == 2) /* count(3) */ > /* branch(end) */ > return do_something (4); /* count(1) */ > result = do_something (8); /* count(2) */ > - break; > + break; /* count(2) */ > default: > result = do_something (32); /* count(1) */ > switch_m++; /* count(1) */ > diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c b/gcc/testsuite/gcc.misc-tests/gcov-4.c > index 9d8ab1c1097..498d299b66b 100644 > --- a/gcc/testsuite/gcc.misc-tests/gcov-4.c > +++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c > @@ -221,7 +221,7 @@ test_switch (int i, int j) > { > case 1: > result = do_something (2); /* count(1) */ > - break; > + break; /* count(1) */ > case 2: > result = do_something (1024); > break; > @@ -230,7 +230,7 @@ test_switch (int i, int j) > if (j == 2) /* count(3) */ > return do_something (4); /* count(1) */ > result = do_something (8); /* count(2) */ > - break; > + break; /* count(2) */ > default: > result = do_something (32); /* count(1) */ > switch_m++; /* count(1) */ > -- > 2.34.0 >
On 13/10/2022 13:39, Richard Biener wrote: > On Tue, Oct 11, 2022 at 2:43 PM Jørgen Kvalsvik > <jorgen.kvalsvik@woven-planet.global> wrote: >> >> The coverage support will under some conditions decide to split edges to >> accurately report coverage. By running the test suite with/without this >> edge splitting a small diff shows up, addressed by this patch, which >> should catch future regressions. >> >> Removing the edge splitting: >> >> $ diff --git a/gcc/profile.cc b/gcc/profile.cc >> --- a/gcc/profile.cc >> +++ b/gcc/profile.cc >> @@ -1244,19 +1244,7 @@ branch_prob (bool thunk) >> Don't do that when the locuses match, so >> if (blah) goto something; >> is not computed twice. */ >> - if (last >> - && gimple_has_location (last) >> - && !RESERVED_LOCATION_P (e->goto_locus) >> - && !single_succ_p (bb) >> - && (LOCATION_FILE (e->goto_locus) >> - != LOCATION_FILE (gimple_location (last)) >> - || (LOCATION_LINE (e->goto_locus) >> - != LOCATION_LINE (gimple_location (last))))) >> - { >> - basic_block new_bb = split_edge (e); >> - edge ne = single_succ_edge (new_bb); >> - ne->goto_locus = e->goto_locus; >> - } >> + >> if ((e->flags & (EDGE_ABNORMAL | EDGE_ABNORMAL_CALL)) >> && e->dest != EXIT_BLOCK_PTR_FOR_FN (cfun)) >> need_exit_edge = 1; >> >> Assuming the .gcov files from make chec-gcc RUNTESTFLAGS=gcov.exp are >> kept: >> >> $ diff -r no-split-edge with-split-edge | grep -C 2 -E "^[<>]\s\s" >> diff -r sans-split-edge/gcc/gcov-4.c.gcov with-split-edge/gcc/gcov-4.c.gcov >> 228c228 >> < -: 224: break; >> --- >> > 1: 224: break; >> 231c231 >> < -: 227: break; >> --- >> > #####: 227: break; >> 237c237 >> < -: 233: break; >> --- >> > 2: 233: break; >> >> gcc/testsuite/ChangeLog: > > OK. > > Thanks, > Richard. > >> >> * g++.dg/gcov/gcov-1.C: Add line count check. >> * gcc.misc-tests/gcov-4.c: Likewise. >> --- >> gcc/testsuite/g++.dg/gcov/gcov-1.C | 8 ++++---- >> gcc/testsuite/gcc.misc-tests/gcov-4.c | 4 ++-- >> 2 files changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C b/gcc/testsuite/g++.dg/gcov/gcov-1.C >> index 9018b9a3a73..ee383b480a8 100644 >> --- a/gcc/testsuite/g++.dg/gcov/gcov-1.C >> +++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C >> @@ -257,20 +257,20 @@ test_switch (int i, int j) >> switch (i) /* count(5) */ >> /* branch(end) */ >> { >> - case 1: >> + case 1: /* count(1) */ >> result = do_something (2); /* count(1) */ >> - break; >> + break; /* count(1) */ >> case 2: >> result = do_something (1024); >> break; >> - case 3: >> + case 3: /* count(3) */ >> case 4: >> /* branch(67) */ >> if (j == 2) /* count(3) */ >> /* branch(end) */ >> return do_something (4); /* count(1) */ >> result = do_something (8); /* count(2) */ >> - break; >> + break; /* count(2) */ >> default: >> result = do_something (32); /* count(1) */ >> switch_m++; /* count(1) */ >> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c b/gcc/testsuite/gcc.misc-tests/gcov-4.c >> index 9d8ab1c1097..498d299b66b 100644 >> --- a/gcc/testsuite/gcc.misc-tests/gcov-4.c >> +++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c >> @@ -221,7 +221,7 @@ test_switch (int i, int j) >> { >> case 1: >> result = do_something (2); /* count(1) */ >> - break; >> + break; /* count(1) */ >> case 2: >> result = do_something (1024); >> break; >> @@ -230,7 +230,7 @@ test_switch (int i, int j) >> if (j == 2) /* count(3) */ >> return do_something (4); /* count(1) */ >> result = do_something (8); /* count(2) */ >> - break; >> + break; /* count(2) */ >> default: >> result = do_something (32); /* count(1) */ >> switch_m++; /* count(1) */ >> -- >> 2.34.0 >> Thank you, I've installed the patches. I noticed that the inclusion of diffs in the message works fine with git generally, but could be picked up on by git format-patch && git am < patch. I hope that does not end up causing too many problems (last time, promise!) Thanks, Jørgen
diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C b/gcc/testsuite/g++.dg/gcov/gcov-1.C index 9018b9a3a73..ee383b480a8 100644 --- a/gcc/testsuite/g++.dg/gcov/gcov-1.C +++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C @@ -257,20 +257,20 @@ test_switch (int i, int j) switch (i) /* count(5) */ /* branch(end) */ { - case 1: + case 1: /* count(1) */ result = do_something (2); /* count(1) */ - break; + break; /* count(1) */ case 2: result = do_something (1024); break; - case 3: + case 3: /* count(3) */ case 4: /* branch(67) */ if (j == 2) /* count(3) */ /* branch(end) */ return do_something (4); /* count(1) */ result = do_something (8); /* count(2) */ - break; + break; /* count(2) */ default: result = do_something (32); /* count(1) */ switch_m++; /* count(1) */ diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c b/gcc/testsuite/gcc.misc-tests/gcov-4.c index 9d8ab1c1097..498d299b66b 100644 --- a/gcc/testsuite/gcc.misc-tests/gcov-4.c +++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c @@ -221,7 +221,7 @@ test_switch (int i, int j) { case 1: result = do_something (2); /* count(1) */ - break; + break; /* count(1) */ case 2: result = do_something (1024); break; @@ -230,7 +230,7 @@ test_switch (int i, int j) if (j == 2) /* count(3) */ return do_something (4); /* count(1) */ result = do_something (8); /* count(2) */ - break; + break; /* count(2) */ default: result = do_something (32); /* count(1) */ switch_m++; /* count(1) */