Re: [PATCH] Fix assertions along default switch labels (PR tree-optimization/78819)

[Richard Biener <richard dot guenther at gmail dot com>]

On Fri, Dec 16, 2016 at 12:49 PM, Marek Polacek <polacek@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  <polacek@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