[PATCH][PR rtl-optimization/81308] Conditionally cleanup the CFG after switch conversion

[Richard Biener]

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
>