Re: [PATCH] Don't add a new label for implicit default label if case labels cover the whole range (PR middle-end/39666)

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

On Wed, Apr 29, 2009 at 11:44 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> If a SWITCH_EXPR doesn't have a default case, but all its case labels
> together cover all values from type's minimum through maximum, when
> gimplify_switch_expr adds the default case it can introduce undesirable
> warnings and pessimize generated code.
> As the case labels are sorted, it is enough to check that first case label
> starts at TYPE_MIN_VALUE and last ends at TYPE_MAX_VALUE (to bail out quickly
> in a typical case) and if so check that adjacent case labels have adjacent
> ranges.
>
> Bootstrapped/regtested on x86_64-linux (except Ada, which is currently
> broken), will commit when slush is over.

Hmmm.  Using TYPE_MIN/MAX_VALUE may have issues with both
enum types in C++ and Ada integer subtypes.

Richard.

> 2009-04-29 ?Jakub Jelinek ?<jakub@redhat.com>
>
> ? ? ? ?PR middle-end/39666
> ? ? ? ?* gimplify.c (gimplify_switch_expr): If case labels cover the whole
> ? ? ? ?range of the type, but default label is missing, add it with one
> ? ? ? ?of the existing labels instead of adding a new label for it.
>
> ? ? ? ?* gcc.dg/pr39666-1.c: New test.
> ? ? ? ?* gcc.dg/pr39666-2.c: Likewise.
> ? ? ? ?* g++.dg/warn/Wuninitialized-4.C: Likewise.
> ? ? ? ?* g++.dg/warn/Wuninitialized-5.C: Likewise.
> ? ? ? ?* gfortran.dg/pr39666-1.f90: Likewise.
> ? ? ? ?* gfortran.dg/pr39666-2.f90: Likewise.
>
> --- gcc/gimplify.c.jj ? 2009-04-17 15:03:51.000000000 +0200
> +++ gcc/gimplify.c ? ? ?2009-04-29 15:25:40.000000000 +0200
> @@ -1599,20 +1599,63 @@ gimplify_switch_expr (tree *expr_p, gimp
> ? ? ? ?}
> ? ? ? len = i;
>
> + ? ? ?if (!VEC_empty (tree, labels))
> + ? ? ? sort_case_labels (labels);
> +
> ? ? ? if (!default_case)
> ? ? ? ?{
> - ? ? ? ? gimple new_default;
> + ? ? ? ? tree type = TREE_TYPE (switch_expr);
>
> ? ? ? ? ?/* If the switch has no default label, add one, so that we jump
> - ? ? ? ? ? ?around the switch body. ?*/
> - ? ? ? ? default_case = build3 (CASE_LABEL_EXPR, void_type_node, NULL_TREE,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?NULL_TREE, create_artificial_label ());
> - ? ? ? ? new_default = gimple_build_label (CASE_LABEL (default_case));
> - ? ? ? ? gimplify_seq_add_stmt (&switch_body_seq, new_default);
> - ? ? ? }
> + ? ? ? ? ? ?around the switch body. ?If the labels already cover the whole
> + ? ? ? ? ? ?range of type, add the default label pointing to one of the
> + ? ? ? ? ? ?existing labels. ?*/
> + ? ? ? ? if (type == void_type_node)
> + ? ? ? ? ? type = TREE_TYPE (SWITCH_COND (switch_expr));
> + ? ? ? ? if (len
> + ? ? ? ? ? ? && INTEGRAL_TYPE_P (type)
> + ? ? ? ? ? ? && TYPE_MIN_VALUE (type)
> + ? ? ? ? ? ? && TYPE_MAX_VALUE (type)
> + ? ? ? ? ? ? && tree_int_cst_equal (CASE_LOW (VEC_index (tree, labels, 0)),
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?TYPE_MIN_VALUE (type)))
> + ? ? ? ? ? {
> + ? ? ? ? ? ? tree low, high = CASE_HIGH (VEC_index (tree, labels, len - 1));
> + ? ? ? ? ? ? if (!high)
> + ? ? ? ? ? ? ? high = CASE_LOW (VEC_index (tree, labels, len - 1));
> + ? ? ? ? ? ? if (tree_int_cst_equal (high, TYPE_MAX_VALUE (type)))
> + ? ? ? ? ? ? ? {
> + ? ? ? ? ? ? ? ? for (i = 1; i < len; i++)
> + ? ? ? ? ? ? ? ? ? {
> + ? ? ? ? ? ? ? ? ? ? high = CASE_LOW (VEC_index (tree, labels, i));
> + ? ? ? ? ? ? ? ? ? ? low = CASE_HIGH (VEC_index (tree, labels, i - 1));
> + ? ? ? ? ? ? ? ? ? ? if (!low)
> + ? ? ? ? ? ? ? ? ? ? ? low = CASE_LOW (VEC_index (tree, labels, i - 1));
> + ? ? ? ? ? ? ? ? ? ? if ((TREE_INT_CST_LOW (low) + 1
> + ? ? ? ? ? ? ? ? ? ? ? ? ?!= TREE_INT_CST_LOW (high))
> + ? ? ? ? ? ? ? ? ? ? ? ? || (TREE_INT_CST_HIGH (low)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? + (TREE_INT_CST_LOW (high) == 0)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? != TREE_INT_CST_HIGH (high)))
> + ? ? ? ? ? ? ? ? ? ? ? break;
> + ? ? ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? ? if (i == len)
> + ? ? ? ? ? ? ? ? ? default_case = build3 (CASE_LABEL_EXPR, void_type_node,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?NULL_TREE, NULL_TREE,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?CASE_LABEL (VEC_index (tree,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? labels, 0)));
> + ? ? ? ? ? ? ? }
> + ? ? ? ? ? }
>
> - ? ? ?if (!VEC_empty (tree, labels))
> - ? ? ? sort_case_labels (labels);
> + ? ? ? ? if (!default_case)
> + ? ? ? ? ? {
> + ? ? ? ? ? ? gimple new_default;
> +
> + ? ? ? ? ? ? default_case = build3 (CASE_LABEL_EXPR, void_type_node,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?NULL_TREE, NULL_TREE,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?create_artificial_label ());
> + ? ? ? ? ? ? new_default = gimple_build_label (CASE_LABEL (default_case));
> + ? ? ? ? ? ? gimplify_seq_add_stmt (&switch_body_seq, new_default);
> + ? ? ? ? ? }
> + ? ? ? }
>
> ? ? ? gimple_switch = gimple_build_switch_vec (SWITCH_COND (switch_expr),
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?default_case, labels);
> --- gcc/testsuite/gcc.dg/pr39666-1.c.jj 2009-04-29 15:40:48.000000000 +0200
> +++ gcc/testsuite/gcc.dg/pr39666-1.c ? ?2009-04-29 15:40:22.000000000 +0200
> @@ -0,0 +1,22 @@
> +/* PR middle-end/39666 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -Wuninitialized" } */
> +
> +int
> +foo (int i)
> +{
> + ?int j;
> + ?switch (i)
> + ? ?{
> + ? ?case -__INT_MAX__ - 1 ... -1:
> + ? ? ?j = 6;
> + ? ? ?break;
> + ? ?case 0:
> + ? ? ?j = 5;
> + ? ? ?break;
> + ? ?case 1 ... __INT_MAX__:
> + ? ? ?j = 4;
> + ? ? ?break;
> + ? ?}
> + ?return j;
> +}
> --- gcc/testsuite/gcc.dg/pr39666-2.c.jj 2009-04-29 15:40:46.000000000 +0200
> +++ gcc/testsuite/gcc.dg/pr39666-2.c ? ?2009-04-29 15:47:13.000000000 +0200
> @@ -0,0 +1,22 @@
> +/* PR middle-end/39666 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -Wuninitialized" } */
> +
> +int
> +foo (int i)
> +{
> + ?int j; ? ? ? /* { dg-warning "may be used uninitialized" } */
> + ?switch (i)
> + ? ?{
> + ? ?case -__INT_MAX__ - 1 ... -1:
> + ? ? ?j = 6;
> + ? ? ?break;
> + ? ?case 0:
> + ? ? ?j = 5;
> + ? ? ?break;
> + ? ?case 2 ... __INT_MAX__:
> + ? ? ?j = 4;
> + ? ? ?break;
> + ? ?}
> + ?return j;
> +}
> --- gcc/testsuite/g++.dg/warn/Wuninitialized-4.C.jj ? ? 2009-04-29 15:42:05.000000000 +0200
> +++ gcc/testsuite/g++.dg/warn/Wuninitialized-4.C ? ? ? ?2009-04-29 15:42:18.000000000 +0200
> @@ -0,0 +1,22 @@
> +// PR middle-end/39666
> +// { dg-do compile }
> +// { dg-options "-O2 -Wuninitialized" }
> +
> +int
> +foo (int i)
> +{
> + ?int j;
> + ?switch (i)
> + ? ?{
> + ? ?case -__INT_MAX__ - 1 ... -1:
> + ? ? ?j = 6;
> + ? ? ?break;
> + ? ?case 0:
> + ? ? ?j = 5;
> + ? ? ?break;
> + ? ?case 1 ... __INT_MAX__:
> + ? ? ?j = 4;
> + ? ? ?break;
> + ? ?}
> + ?return j;
> +}
> --- gcc/testsuite/g++.dg/warn/Wuninitialized-5.C.jj ? ? 2009-04-29 15:42:05.000000000 +0200
> +++ gcc/testsuite/g++.dg/warn/Wuninitialized-5.C ? ? ? ?2009-04-29 15:47:43.000000000 +0200
> @@ -0,0 +1,22 @@
> +// PR middle-end/39666
> +// { dg-do compile }
> +// { dg-options "-O2 -Wuninitialized" }
> +
> +int
> +foo (int i)
> +{
> + ?int j; ? ? ? // { dg-warning "may be used uninitialized" }
> + ?switch (i)
> + ? ?{
> + ? ?case -__INT_MAX__ - 1 ... -1:
> + ? ? ?j = 6;
> + ? ? ?break;
> + ? ?case 0:
> + ? ? ?j = 5;
> + ? ? ?break;
> + ? ?case 2 ... __INT_MAX__:
> + ? ? ?j = 4;
> + ? ? ?break;
> + ? ?}
> + ?return j;
> +}
> --- gcc/testsuite/gfortran.dg/pr39666-1.f90.jj ?2009-04-29 15:42:57.000000000 +0200
> +++ gcc/testsuite/gfortran.dg/pr39666-1.f90 ? ? 2009-04-29 15:43:50.000000000 +0200
> @@ -0,0 +1,14 @@
> +! PR middle-end/39666
> +! { dg-do compile }
> +! { dg-options "-O2 -Wuninitialized" }
> +
> +FUNCTION f(n)
> + ?INTEGER, INTENT(in) :: n
> + ?REAL ? ? ? ? ? ? ? ?:: f
> +
> + ?SELECT CASE (n)
> + ? ?CASE (:-1); f = -1.0
> + ? ?CASE (0); ? f = ?0.0
> + ? ?CASE (1:); ?f = ?1.0
> + ?END SELECT
> +END FUNCTION
> --- gcc/testsuite/gfortran.dg/pr39666-2.f90.jj ?2009-04-29 15:42:57.000000000 +0200
> +++ gcc/testsuite/gfortran.dg/pr39666-2.f90 ? ? 2009-04-29 15:44:46.000000000 +0200
> @@ -0,0 +1,14 @@
> +! PR middle-end/39666
> +! { dg-do compile }
> +! { dg-options "-O2 -Wuninitialized" }
> +
> +FUNCTION f(n) ?! { dg-warning "may be used uninitialized" }
> + ?INTEGER, INTENT(in) :: n
> + ?REAL ? ? ? ? ? ? ? ?:: f
> +
> + ?SELECT CASE (n)
> + ? ?CASE (:-1); f = -1.0
> + ? ?CASE (0); ? f = ?0.0
> + ? ?CASE (2:); ?f = ?1.0
> + ?END SELECT
> +END FUNCTION
>
> ? ? ? ?Jakub
>