Message ID | 19ed28c8-72d1-8ca9-f001-c1e605b38c09@redhat.com |
---|---|
State | New |
Headers | show |
Series | [PR,rtl-optimization/81308] Conditionally cleanup the CFG after switch conversion | expand |
On Mon, Jan 8, 2018 at 5:45 AM, Jeff Law <law@redhat.com> wrote: > This patch fixes the second testcase in 81308 and the duplicate in 83724. > > For those cases we have a switch statement where one or more case labels > are marked as __builtin_unreachable. > > Switch conversion calls group_case_labels which can drop the edges from > the switch to the case labels that are marked as __builtin_unreachable. > > That leaves those blocks unreachable and thus we again trigger the > assertion failure in the dominance code. > > My solution here is again to note if a change was made that ought to > trigger a CFG cleanup (group_case_labels returns a suitable boolean). > If such a change was made then the pass returns TODO_cleanup_cfg. > > Bootstrapped and regression tested on x86_64. OK for the trunk? Ok. RIchard. > Jeff > > > PR rtl-optimizatin/81308 > * tree-switch-conversion.c (cfg_altered): New file scoped static. > (process_switch): If group_case_labels makes a change, then set > cfg_altered. > (pass_convert_switch::execute): If a switch is converted, then > set cfg_altered. Return TODO_cfg_cleanup if cfg_altered is true. > > > PR rtl-optimizatin/81308 > * g++.dg/pr81308-1.C: New test. > * g++.dg/pr81308-2.C: New test. > > diff --git a/gcc/testsuite/g++.dg/pr81308-1.C b/gcc/testsuite/g++.dg/pr81308-1.C > new file mode 100644 > index 0000000..508372b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/pr81308-1.C > @@ -0,0 +1,67 @@ > +/* { dg-do compile } */ > +/* { dg-options "-w -O2 -fno-exceptions -std=c++11 -fpermissive" } */ > + > +namespace a { > +template <typename b, b c> struct d { static constexpr b e = c; }; > +template <typename> struct f : d<bool, __is_trivially_copyable(int)> {}; > +} > +typedef long g; > +template <typename> struct h { static const bool e = a::f<int>::e; }; > +namespace a { > +template <typename> struct ah; > +template <typename> class ai; > +} > +class i { > +public: > + operator[](long) const {} > +}; > +template <typename, int> class am : public i {}; > +class an; > +class k : public am<a::ai<an>, h<a::ai<a::ah<an>>>::e> {}; > +class l { > +public: > + aq(); > +}; > +class ar extern as; > +typedef k at; > +class m { > + virtual bool av(int, unsigned &, at &, int &, g &, bool); > +}; > +class ar { > +public: > + typedef m *aw(const &, int &, const &, const &); > +}; > +struct ax { > + static ay(ar::aw); > +}; > +template <class az> struct n { > + n(ar) { ax::ay(ba); } > + static m *ba(const &bb, int &bc, const &bd, const &be) { az(bb, bc, bd, be); } > +}; > +namespace { > +class G : m { > + unsigned bi(const at &, l &); > + bool av(int, unsigned &, at &, int &, g &, bool); > + > +public: > + G(const, int, const, const) {} > +}; > +} > +bool G::av(int, unsigned &, at &bl, int &, g &, bool) { > + l bo; > + bi(bl, bo); > +} > +o() { n<G> bp(as); } > +namespace { > +enum { bq, br }; > +} > +unsigned G::bi(const at &bl, l &bo) { > + unsigned bs; > + for (char *j;; j += 2) > + switch (*j) { > + case bq: > + bl[bs]; > + case br: > + bo.aq(); > + } > +} > diff --git a/gcc/testsuite/g++.dg/pr81308-2.C b/gcc/testsuite/g++.dg/pr81308-2.C > new file mode 100644 > index 0000000..97e3409 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/pr81308-2.C > @@ -0,0 +1,38 @@ > +/* { dg-do compile } */ > +/* { dg-options "-w -O2" } */ > + > +struct A { > + int operator[](int) const {} > +}; > +struct B { > + void m_fn1(); > +}; > +struct C { > + virtual bool m_fn2(int, unsigned &, A &, int &, unsigned long &, bool); > +}; > +template <class MCAsmParserImpl> struct D { > + D(int) { MCAsmParserImpl(0, 0, 0, 0); } > +}; > +int a; > +namespace { > +struct F : C { > + bool m_fn2(int, unsigned &, A &, int &, unsigned long &, bool); > + unsigned m_fn3(const A &, B &); > + F(int, int, int, int) {} > +}; > +} > +bool F::m_fn2(int, unsigned &, A &p3, int &, unsigned long &, bool) { > + B b; > + m_fn3(p3, b); > +} > +void fn1() { D<F>(0); } > +unsigned F::m_fn3(const A &p1, B &p2) { > + for (int *p;; p++) > + switch (*p) { > + case 0: > + p1[a]; > + case 1: > + p2.m_fn1(); > + } > +} > + > diff --git a/gcc/tree-switch-conversion.c b/gcc/tree-switch-conversion.c > index fdec59e..b384e4d 100644 > --- a/gcc/tree-switch-conversion.c > +++ b/gcc/tree-switch-conversion.c > @@ -60,6 +60,10 @@ Software Foundation, 51 Franklin Street, Fifth Floor, Boston, MA > targetm.case_values_threshold(), or be its own param. */ > #define MAX_CASE_BIT_TESTS 3 > > +/* Track whether or not we have altered the CFG and thus may need to > + cleanup the CFG when complete. */ > +bool cfg_altered; > + > /* Split the basic block at the statement pointed to by GSIP, and insert > a branch to the target basic block of E_TRUE conditional on tree > expression COND. > @@ -1492,7 +1496,7 @@ process_switch (gswitch *swtch) > > /* Group case labels so that we get the right results from the heuristics > that decide on the code generation approach for this switch. */ > - group_case_labels_stmt (swtch); > + cfg_altered |= group_case_labels_stmt (swtch); > > /* If this switch is now a degenerate case with only a default label, > there is nothing left for us to do. */ > @@ -1605,6 +1609,7 @@ pass_convert_switch::execute (function *fun) > { > basic_block bb; > > + cfg_altered = false; > FOR_EACH_BB_FN (bb, fun) > { > const char *failure_reason; > @@ -1625,6 +1630,7 @@ pass_convert_switch::execute (function *fun) > failure_reason = process_switch (as_a <gswitch *> (stmt)); > if (! failure_reason) > { > + cfg_altered = true; > if (dump_file) > { > fputs ("Switch converted\n", dump_file); > @@ -1648,7 +1654,7 @@ pass_convert_switch::execute (function *fun) > } > } > > - return 0; > + return cfg_altered ? TODO_cleanup_cfg : 0; > } > > } // anon namespace >
diff --git a/gcc/testsuite/g++.dg/pr81308-1.C b/gcc/testsuite/g++.dg/pr81308-1.C new file mode 100644 index 0000000..508372b --- /dev/null +++ b/gcc/testsuite/g++.dg/pr81308-1.C @@ -0,0 +1,67 @@ +/* { dg-do compile } */ +/* { dg-options "-w -O2 -fno-exceptions -std=c++11 -fpermissive" } */ + +namespace a { +template <typename b, b c> struct d { static constexpr b e = c; }; +template <typename> struct f : d<bool, __is_trivially_copyable(int)> {}; +} +typedef long g; +template <typename> struct h { static const bool e = a::f<int>::e; }; +namespace a { +template <typename> struct ah; +template <typename> class ai; +} +class i { +public: + operator[](long) const {} +}; +template <typename, int> class am : public i {}; +class an; +class k : public am<a::ai<an>, h<a::ai<a::ah<an>>>::e> {}; +class l { +public: + aq(); +}; +class ar extern as; +typedef k at; +class m { + virtual bool av(int, unsigned &, at &, int &, g &, bool); +}; +class ar { +public: + typedef m *aw(const &, int &, const &, const &); +}; +struct ax { + static ay(ar::aw); +}; +template <class az> struct n { + n(ar) { ax::ay(ba); } + static m *ba(const &bb, int &bc, const &bd, const &be) { az(bb, bc, bd, be); } +}; +namespace { +class G : m { + unsigned bi(const at &, l &); + bool av(int, unsigned &, at &, int &, g &, bool); + +public: + G(const, int, const, const) {} +}; +} +bool G::av(int, unsigned &, at &bl, int &, g &, bool) { + l bo; + bi(bl, bo); +} +o() { n<G> bp(as); } +namespace { +enum { bq, br }; +} +unsigned G::bi(const at &bl, l &bo) { + unsigned bs; + for (char *j;; j += 2) + switch (*j) { + case bq: + bl[bs]; + case br: + bo.aq(); + } +} diff --git a/gcc/testsuite/g++.dg/pr81308-2.C b/gcc/testsuite/g++.dg/pr81308-2.C new file mode 100644 index 0000000..97e3409 --- /dev/null +++ b/gcc/testsuite/g++.dg/pr81308-2.C @@ -0,0 +1,38 @@ +/* { dg-do compile } */ +/* { dg-options "-w -O2" } */ + +struct A { + int operator[](int) const {} +}; +struct B { + void m_fn1(); +}; +struct C { + virtual bool m_fn2(int, unsigned &, A &, int &, unsigned long &, bool); +}; +template <class MCAsmParserImpl> struct D { + D(int) { MCAsmParserImpl(0, 0, 0, 0); } +}; +int a; +namespace { +struct F : C { + bool m_fn2(int, unsigned &, A &, int &, unsigned long &, bool); + unsigned m_fn3(const A &, B &); + F(int, int, int, int) {} +}; +} +bool F::m_fn2(int, unsigned &, A &p3, int &, unsigned long &, bool) { + B b; + m_fn3(p3, b); +} +void fn1() { D<F>(0); } +unsigned F::m_fn3(const A &p1, B &p2) { + for (int *p;; p++) + switch (*p) { + case 0: + p1[a]; + case 1: + p2.m_fn1(); + } +} + diff --git a/gcc/tree-switch-conversion.c b/gcc/tree-switch-conversion.c index fdec59e..b384e4d 100644 --- a/gcc/tree-switch-conversion.c +++ b/gcc/tree-switch-conversion.c @@ -60,6 +60,10 @@ Software Foundation, 51 Franklin Street, Fifth Floor, Boston, MA targetm.case_values_threshold(), or be its own param. */ #define MAX_CASE_BIT_TESTS 3 +/* Track whether or not we have altered the CFG and thus may need to + cleanup the CFG when complete. */ +bool cfg_altered; + /* Split the basic block at the statement pointed to by GSIP, and insert a branch to the target basic block of E_TRUE conditional on tree expression COND. @@ -1492,7 +1496,7 @@ process_switch (gswitch *swtch) /* Group case labels so that we get the right results from the heuristics that decide on the code generation approach for this switch. */ - group_case_labels_stmt (swtch); + cfg_altered |= group_case_labels_stmt (swtch); /* If this switch is now a degenerate case with only a default label, there is nothing left for us to do. */ @@ -1605,6 +1609,7 @@ pass_convert_switch::execute (function *fun) { basic_block bb; + cfg_altered = false; FOR_EACH_BB_FN (bb, fun) { const char *failure_reason; @@ -1625,6 +1630,7 @@ pass_convert_switch::execute (function *fun) failure_reason = process_switch (as_a <gswitch *> (stmt)); if (! failure_reason) { + cfg_altered = true; if (dump_file) { fputs ("Switch converted\n", dump_file); @@ -1648,7 +1654,7 @@ pass_convert_switch::execute (function *fun) } } - return 0; + return cfg_altered ? TODO_cleanup_cfg : 0; } } // anon namespace