Message ID | 20191121095625.GF4650@tucnak |
---|---|
State | New |
Headers | show |
Series | Fix up sink select_best_block (PR tree-optimization/91355) | expand |
On Thu, 21 Nov 2019, Jakub Jelinek wrote: > Hi! > > This patch doesn't fix the actual bug somewhere else, where inserting > stmts on the edge between resx and corresponding landing pad confuses > ehcleanup2, but attempts to restore previous behavior in the heuristics, > which is IMHO desirable too (and makes the bug latent). > r254698 changed: > if (bb_loop_depth (best_bb) == bb_loop_depth (early_bb) > - && best_bb->count.to_frequency (cfun) > - < (early_bb->count.to_frequency (cfun) * threshold / 100.0)) > + /* If result of comparsion is unknown, preffer EARLY_BB. > + Thus use !(...>=..) rather than (...<...) */ > + && !(best_bb->count.apply_scale (100, 1) > + > (early_bb->count.apply_scale (threshold, 1)))) > return best_bb; > I understand the difference between x < y and !(x >= y), profile_count > comparisons are partial ordering, but the old x < y corresponds to > !(x >= y) as the comment says, not to !(x > y) as the patch implemented. > On the testcase, we have > something; > resx 4; > and the count on the resx 4; bb is already zero, it is already something > invoked after first throw, there is no advantage to sink statements across > the resx 4; to the fallthru (across EH edge) bb, because both bbs will be > executed exactly the same times. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? OK. > 2019-11-21 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/91355 > * tree-ssa-sink.c (select_best_block): Use >= rather than > > for early_bb scaled count with best_bb count comparison. > > * g++.dg/torture/pr91355.C: New test. > > --- gcc/tree-ssa-sink.c.jj 2019-11-20 14:25:19.081593017 +0100 > +++ gcc/tree-ssa-sink.c 2019-11-20 15:44:13.763686288 +0100 > @@ -228,7 +228,7 @@ select_best_block (basic_block early_bb, > /* If result of comparsion is unknown, prefer EARLY_BB. > Thus use !(...>=..) rather than (...<...) */ > && !(best_bb->count.apply_scale (100, 1) > - > (early_bb->count.apply_scale (threshold, 1)))) > + >= early_bb->count.apply_scale (threshold, 1))) > return best_bb; > > /* No better block found, so return EARLY_BB, which happens to be the > --- gcc/testsuite/g++.dg/torture/pr91355.C.jj 2019-11-20 17:21:49.428962914 +0100 > +++ gcc/testsuite/g++.dg/torture/pr91355.C 2019-11-20 17:23:46.370210313 +0100 > @@ -0,0 +1,28 @@ > +// PR tree-optimization/91355 > +// { dg-do run } > +// { dg-options "-std=c++14" } > + > +unsigned int d = 0; > + > +struct S { > + S () { d++; } > + S (const S &) { d++; } > + ~S () { d--; } > +}; > + > +void > +foo (int i) throw (int) // { dg-warning "dynamic exception specifications are deprecated" } > +{ > + if (i == 0) > + throw 3; > + S d; > + throw 3; > +} > + > +int > +main () > +{ > + try { foo (1); } catch (...) {} > + if (d) > + __builtin_abort (); > +} > > Jakub > >
> Hi! > > This patch doesn't fix the actual bug somewhere else, where inserting > stmts on the edge between resx and corresponding landing pad confuses > ehcleanup2, but attempts to restore previous behavior in the heuristics, > which is IMHO desirable too (and makes the bug latent). > r254698 changed: > if (bb_loop_depth (best_bb) == bb_loop_depth (early_bb) > - && best_bb->count.to_frequency (cfun) > - < (early_bb->count.to_frequency (cfun) * threshold / 100.0)) > + /* If result of comparsion is unknown, preffer EARLY_BB. > + Thus use !(...>=..) rather than (...<...) */ > + && !(best_bb->count.apply_scale (100, 1) > + > (early_bb->count.apply_scale (threshold, 1)))) > return best_bb; > I understand the difference between x < y and !(x >= y), profile_count > comparisons are partial ordering, but the old x < y corresponds to > !(x >= y) as the comment says, not to !(x > y) as the patch implemented. > On the testcase, we have > something; > resx 4; > and the count on the resx 4; bb is already zero, it is already something > invoked after first throw, there is no advantage to sink statements across > the resx 4; to the fallthru (across EH edge) bb, because both bbs will be > executed exactly the same times. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2019-11-21 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/91355 > * tree-ssa-sink.c (select_best_block): Use >= rather than > > for early_bb scaled count with best_bb count comparison. > > * g++.dg/torture/pr91355.C: New test. This is OK, thanks! Honza
--- gcc/tree-ssa-sink.c.jj 2019-11-20 14:25:19.081593017 +0100 +++ gcc/tree-ssa-sink.c 2019-11-20 15:44:13.763686288 +0100 @@ -228,7 +228,7 @@ select_best_block (basic_block early_bb, /* If result of comparsion is unknown, prefer EARLY_BB. Thus use !(...>=..) rather than (...<...) */ && !(best_bb->count.apply_scale (100, 1) - > (early_bb->count.apply_scale (threshold, 1)))) + >= early_bb->count.apply_scale (threshold, 1))) return best_bb; /* No better block found, so return EARLY_BB, which happens to be the --- gcc/testsuite/g++.dg/torture/pr91355.C.jj 2019-11-20 17:21:49.428962914 +0100 +++ gcc/testsuite/g++.dg/torture/pr91355.C 2019-11-20 17:23:46.370210313 +0100 @@ -0,0 +1,28 @@ +// PR tree-optimization/91355 +// { dg-do run } +// { dg-options "-std=c++14" } + +unsigned int d = 0; + +struct S { + S () { d++; } + S (const S &) { d++; } + ~S () { d--; } +}; + +void +foo (int i) throw (int) // { dg-warning "dynamic exception specifications are deprecated" } +{ + if (i == 0) + throw 3; + S d; + throw 3; +} + +int +main () +{ + try { foo (1); } catch (...) {} + if (d) + __builtin_abort (); +}