On 2023/3/2 16:41, Richard Biener wrote:
On Thu, Mar 2, 2023 at 3:31 AM Xionghu Luo via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
When spliting edge with self loop, the split edge should be placed just next to
the edge_in->src, otherwise it may generate different position latch bbs for
two consecutive self loops. For details, please refer to:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93680#c4
Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for
master?
gcc/ChangeLog:
PR gcov/93680
* tree-cfg.cc (split_edge_bb_loc): Return edge_in->src for self loop.
gcc/testsuite/ChangeLog:
PR gcov/93680
* gcc.misc-tests/gcov-pr93680.c: New test.
Signed-off-by: Xionghu Luo <xionghu...@tencent.com>
---
gcc/testsuite/gcc.misc-tests/gcov-pr93680.c | 24 +++++++++++++++++++++
gcc/tree-cfg.cc | 2 +-
2 files changed, 25 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
new file mode 100644
index 00000000000..b2bf9e626fc
--- /dev/null
+++ b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
@@ -0,0 +1,24 @@
+/* { dg-options "-fprofile-arcs -ftest-coverage" } */
+/* { dg-do run { target native } } */
+
+int f(int s, int n)
+{
+ int p = 0;
+
+ switch (s)
+ {
+ case 0: /* count(5) */
+ do { p++; } while (--n); /* count(5) */
+ return p; /* count(1) */
+
+ case 1: /* count(5) */
+ do { p++; } while (--n); /* count(5) */
+ return p; /* count(1) */
+ }
+
+ return 0;
+}
+
+int main() { f(0, 5); f(1, 5); return 0; }
+
+/* { dg-final { run-gcov gcov-pr93680.c } } */
diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
index a9fcc7fd050..6fa1d83d366 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -3009,7 +3009,7 @@ split_edge_bb_loc (edge edge_in)
if (dest_prev)
{
edge e = find_edge (dest_prev, dest);
- if (e && !(e->flags & EDGE_COMPLEX))
+ if ((e && !(e->flags & EDGE_COMPLEX)) || edge_in->src == edge_in->dest)
I think this should eventually apply to all backedge edge_in, correct?
But of course
we cannot easily test for this here.
Still since this affects ordering in the {next,prev}_bb chain only but not CFG
semantics I wonder how it can affect coverage? Isn't it only by chance that
this block order survives?
For case:
1 int f(int s, int n)
2 {
3 int p = 0;
4 int q = 0;
5
6 switch (s)
7 {
8 case 0:
9 do { p++; } while (--n);
10 return p;
11
12 case 1:
13 do { p++; } while (--n);
14 return p;
15 }
16
17 return 0;
18 }
19
20 int main() { f(0, 5); f(1, 5);}
current GCC generates:
<bb 2> :
...
<bb 3> : <= first loop
...
goto <bb 4>; [INV]
else
goto <bb 5>; [INV]
<bb 4> : <= first latch bb
goto <bb 3>; [100.00%]
<bb 5> :
...
goto <bb 10>; [INV]
<bb 6> : <= second latch bb
<bb 7> : <= second loop
...
goto <bb 6>; [INV]
else
goto <bb 8>; [INV]
<bb 4> and <bb 6> are created by split_edge->split_edge_bb_loc, <bb 4>
is located after the loop, but <bb 6> is located before the loop.
First call of split_edge_bb_loc, the dest_prev is <bb 2>, and find_edge
did find a edge from <bb 2> to <bb 3>, the returned afte_bb is <bb 3>, so
latch <bb 4> is put after the loop
but second call of split_edge_bb_loc, the dest_prev is <bb 5>, so find_edge
return 0, and the returned after_bb is <bb 5>, then the created latch <bb 6>
is put before the loop...
Different latch bb position caused different gcno, while gcov has poor
information and not that smart to recognize it:(, is it reasonable to keep
this kind of loops same order?
small.gcno: 648: block 2:`small.c':1, 3, 4, 6
small.gcno: 688: 01450000: 36:LINES
small.gcno: 700: block 3:`small.c':8, 9
small.gcno: 732: 01450000: 32:LINES
small.gcno: 744: block 5:`small.c':10
-small.gcno: 772: 01450000: 32:LINES
-small.gcno: 784: block 6:`small.c':12
-small.gcno: 812: 01450000: 36:LINES
-small.gcno: 824: block 7:`small.c':12, 13
+small.gcno: 772: 01450000: 36:LINES
+small.gcno: 784: block 6:`small.c':12, 13
+small.gcno: 816: 01450000: 32:LINES
+small.gcno: 828: block 8:`small.c':14
small.gcno: 856: 01450000: 32:LINES
-small.gcno: 868: block 8:`small.c':14
-small.gcno: 896: 01450000: 32:LINES
-small.gcno: 908: block 9:`small.c':17
+small.gcno: 868: block 9:`small.c':17
For the case when both edge_in->src has more than one successor and
edge_in->dest has more than one predecessor there isn't any good heuristic
to make printing the blocks in chain order "nice" (well, the backedge
one maybe).
But as said - this order shouldn't have any effect on semantics ...
return edge_in->src;
}
return dest_prev;
--
2.27.0
