On Fri, Dec 16, 2016 at 12:49 PM, Marek Polacek <pola...@redhat.com> wrote: > Starting with r238761, we register assertions for default switch labels; > these assertions correspond to the anti-range of each case label. It > means that we can optimize this better: > > switch (i) { > case 1: case 2: case 3: > ... > break; > default: > // i is ~[1, 3] > if (i == 2) // fold to 0 > ... > } > > But as this testcase shows, this breaks when the default label shares a label > with another case. On this testcase, when we reach the switch, we know that > argc is either 1, 2, or 3. So by the time we reach vrp2, the IR will have > been optimized to > > switch (argc) { > default: > case 3: > // argc will be considered 1 despite the case 3 > break; > case 2: > ... > } > > Now, the assertion for argc in default: is that it's ~[2,3]. Since argc had > been proved to be either 1, 2, or 3, we optimized it to 1. Given the > consecutive case 3: this is incorrect. > > So ignore the range when a case label is "shared" with the default case. > > Another bug: --param max-vrp-switch-assertions=0 didn't work -- we register > the assertion and then there's this check > > if (--insertion_limit == 0) > break; > > but given the decrement this isn't true so we'd kept going. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2016-12-16 Marek Polacek <pola...@redhat.com> > > PR tree-optimization/78819 > * tree-vrp.c (find_switch_asserts): Return if the insertion limit is > 0. > Don't register an assertion if the default case shares a label with > another case. > > * gcc.dg/tree-ssa/vrp112.c: New test. > > diff --git gcc/testsuite/gcc.dg/tree-ssa/vrp112.c > gcc/testsuite/gcc.dg/tree-ssa/vrp112.c > index e69de29..fdc6711 100644 > --- gcc/testsuite/gcc.dg/tree-ssa/vrp112.c > +++ gcc/testsuite/gcc.dg/tree-ssa/vrp112.c > @@ -0,0 +1,31 @@ > +/* PR tree-optimization/78819 */ > +/* { dg-do run } */ > +/* { dg-options "-O2" } */ > + > +__attribute__((noinline, noclone)) void > +foo (int argc) > +{ > + if (argc <= 0 || argc > 3) > + return; > + > + switch (argc) > + { > + case 1: > + case 3: > + if (argc != 3) > + __builtin_abort (); > + break; > + case 2: > + asm (""); > + break; > + default: > + __builtin_abort (); > + } > +} > + > +int > +main (void) > +{ > + foo (3); > + return 0; > +} > diff --git gcc/tree-vrp.c gcc/tree-vrp.c > index 97e9953..405469a 100644 > --- gcc/tree-vrp.c > +++ gcc/tree-vrp.c > @@ -6051,10 +6051,17 @@ find_switch_asserts (basic_block bb, gswitch *last) > /* Now register along the default label assertions that correspond to the > anti-range of each label. */ > int insertion_limit = PARAM_VALUE (PARAM_MAX_VRP_SWITCH_ASSERTIONS); > + if (insertion_limit == 0) > + return; > + > + /* We can't do this if the default case shares a label with another case. > */ > + tree default_cl = gimple_switch_default_label (last); > for (idx = 1; idx < n; idx++) > { > tree min, max; > tree cl = gimple_switch_label (last, idx); > + if (CASE_LABEL (cl) == CASE_LABEL (default_cl)) > + break;
It's conservative to break here but don't you actually want to continue instead? > min = CASE_LOW (cl); > max = CASE_HIGH (cl); > @@ -6065,6 +6072,8 @@ find_switch_asserts (basic_block bb, gswitch *last) > { > tree next_min, next_max; > tree next_cl = gimple_switch_label (last, idx); > + if (CASE_LABEL (next_cl) == CASE_LABEL (default_cl)) > + break; here the break is of course correct. Ok with that change. Thanks, Richard. > next_min = CASE_LOW (next_cl); > next_max = CASE_HIGH (next_cl); > > Marek