This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][PR rtl-optimization/81308] Conditionally cleanup the CFG after switch conversion
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Jeff Law <law at redhat dot com>
- Cc: gcc-patches <gcc-patches at gcc dot gnu dot org>
- Date: Mon, 8 Jan 2018 12:42:24 +0100
- Subject: Re: [PATCH][PR rtl-optimization/81308] Conditionally cleanup the CFG after switch conversion
- Authentication-results: sourceware.org; auth=none
- References: <19ed28c8-72d1-8ca9-f001-c1e605b38c09@redhat.com>
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
>