Re: [google] Add -fstrict-enum-precision flag (issue4433083)

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

On Thu, Apr 28, 2011 at 9:50 PM, Diego Novillo <dnovillo@google.com> wrote:
>
> This patch from Silvius Rus adds a new flag (-fstrict-enum-precision).
> While porting the patch to 4.6, I noticed that the C++ FE now has a
> similar flag that seems to have similar semantics (-fstrict-enums).
>
> Silvius's patch is used to disable some switch() optimizations that
> assume enum types can *only* take values within that enum (Silvius,
> please correct me if I got this wrong).
>
> Jason, your -fstrict-enums only applies to the C++ front end.
> Silvius's variant affects VRP and gimplification of switches. ?Would
> it be better if we simply moved -fstrict-enums to common options and
> used that to decide whether to optimize switches in VRP and the
> gimplifier?

I think only the C++ frontend suffers from the "problem".  See also
the patch to disable use of this kind of info from VRP.

Richard.

> We use it internally to disable this optimization for code that
> generates values outside of enum ranges.
>
> Committed to google/main. ?Jason, Silvius, what do you think would be
> the best approach to merge this into trunk?
>
> Thanks. ?Diego.
>
> 2011-04-27 ?Silvius Rus ?<silvius.rus@gmail.com>
>
> ? ? ? ?* doc/invoke.texi (fno-strict-enum-precision): Document.
> ? ? ? ?* gimplify.c (gimplify_switch_expr): If
> ? ? ? ?-fno-strict-enum-precision is given, do not consider enum
> ? ? ? ?types.
> ? ? ? ?* tree-vrp.c (stmt_interesting_for_vrp): If
> ? ? ? ?-fno-strict-enum-precision is given, do not look at enum
> ? ? ? ?types.
>
> 2011-04-27 ?Silvius Rus ?<silvius.rus@gmail.com>
>
> ? ? ? ?* g++.dg/other/no-strict-enum-precision-1.C: New.
> ? ? ? ?* g++.dg/other/no-strict-enum-precision-2.C: New.
> ? ? ? ?* g++.dg/other/no-strict-enum-precision-3.C: New.
>
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index cb04d91..cffc70d 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -393,8 +393,8 @@ Objective-C and Objective-C++ Dialects}.
> ?-fsel-sched-pipelining -fsel-sched-pipelining-outer-loops @gol
> ?-fsignaling-nans -fsingle-precision-constant -fsplit-ivs-in-unroller @gol
> ?-fsplit-wide-types -fstack-protector -fstack-protector-all @gol
> --fstrict-aliasing -fstrict-overflow -fthread-jumps -ftracer @gol
> --ftree-bit-ccp @gol
> +-fstrict-aliasing -fstrict-overflow -fno-strict-enum-precision -fthread-jumps
> +-ftracer -ftree-bit-ccp @gol
> ?-ftree-builtin-call-dce -ftree-ccp -ftree-ch -ftree-copy-prop @gol
> ?-ftree-copyrename -ftree-dce -ftree-dominator-opts -ftree-dse @gol
> ?-ftree-forwprop -ftree-fre -ftree-loop-if-convert @gol
> @@ -2073,6 +2073,11 @@ represented in the minimum number of bits needed to represent all the
> ?enumerators). ?This assumption may not be valid if the program uses a
> ?cast to convert an arbitrary integer value to the enumeration type.
>
> +@item -fno-strict-enum-precision
> +@opindex fno-strict-enum-precision
> +Do not perform optimizations of switch() statements based on the
> +precision of enum types.
> +
> ?@item -ftemplate-depth=@var{n}
> ?@opindex ftemplate-depth
> ?Set the maximum instantiation depth for template classes to @var{n}.
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index 83eaedc..8984d39 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -1602,6 +1602,8 @@ gimplify_switch_expr (tree *expr_p, gimple_seq *pre_p)
> ? ? ? ? ? ?type = TREE_TYPE (SWITCH_COND (switch_expr));
> ? ? ? ? ?if (len
> ? ? ? ? ? ? ?&& INTEGRAL_TYPE_P (type)
> + ? ? ? ? ? ? ?&& (flag_strict_enum_precision
> + ? ? ? ? ? ? ? ? ?|| TREE_CODE (type) != ENUMERAL_TYPE)
> ? ? ? ? ? ? ?&& TYPE_MIN_VALUE (type)
> ? ? ? ? ? ? ?&& TYPE_MAX_VALUE (type)
> ? ? ? ? ? ? ?&& tree_int_cst_equal (CASE_LOW (VEC_index (tree, labels, 0)),
> diff --git a/gcc/testsuite/g++.dg/other/no-strict-enum-precision-1.C b/gcc/testsuite/g++.dg/other/no-strict-enum-precision-1.C
> new file mode 100644
> index 0000000..87f263c
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/other/no-strict-enum-precision-1.C
> @@ -0,0 +1,22 @@
> +/* { dg-do run } */
> +/* { dg-options "-fno-strict-enum-precision" } */
> +
> +enum zero_one { zero = 0, one = 1 };
> +
> +int* allocate_bool(zero_one e) {
> + ?int* v = 0;
> + ?switch (e) {
> + ? ?case zero: v = new int(0);
> + ? ?case one: v = new int(1);
> + ?}
> + ?return v;
> +}
> +
> +int main() {
> + ?if (allocate_bool(static_cast<zero_one>(999))) {
> + ? ?/* Error: should not have matched any case label. ?*/
> + ? ?return 1;
> + ?} else {
> + ? ?return 0;
> + ?}
> +}
> diff --git a/gcc/testsuite/g++.dg/other/no-strict-enum-precision-2.C b/gcc/testsuite/g++.dg/other/no-strict-enum-precision-2.C
> new file mode 100644
> index 0000000..5b6af17
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/other/no-strict-enum-precision-2.C
> @@ -0,0 +1,26 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fno-strict-enum-precision" } */
> +
> +enum X {
> + ?X1,
> + ?X2
> +};
> +
> +int foo (enum X x) {
> + ?switch (x) {
> + ? ?case X1:
> + ? ? ?return 0;
> + ? ?case X2:
> + ? ? ?return 1;
> + ?}
> + ?return x;
> +}
> +
> +int main(int argc, char *argv[]) {
> + ?int n = argc + 999;
> + ?if (n == foo(static_cast<X>(n))) {
> + ? ?return 0;
> + ?} else {
> + ? ?return 1;
> + ?}
> +}
> diff --git a/gcc/testsuite/g++.dg/other/no-strict-enum-precision-3.C b/gcc/testsuite/g++.dg/other/no-strict-enum-precision-3.C
> new file mode 100755
> index 0000000..c3802a8
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/other/no-strict-enum-precision-3.C
> @@ -0,0 +1,14 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fno-strict-enum-precision" } */
> +
> +enum X {
> + X1,
> + X2
> +};
> +
> +int main(int argc, char *argv[]) {
> + X x = static_cast<X>(argc + 999);
> + if (x == X1) return 1;
> + if (x == X2) return 1;
> + return 0;
> +}
> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
> index 65d249f..d828a8d 100644
> --- a/gcc/tree-vrp.c
> +++ b/gcc/tree-vrp.c
> @@ -5553,7 +5553,9 @@ stmt_interesting_for_vrp (gimple stmt)
> ? ? ? ? ?&& ((is_gimple_call (stmt)
> ? ? ? ? ? ? ? && gimple_call_fndecl (stmt) != NULL_TREE
> ? ? ? ? ? ? ? && DECL_IS_BUILTIN (gimple_call_fndecl (stmt)))
> - ? ? ? ? ? ? || !gimple_vuse (stmt)))
> + ? ? ? ? ? ? || !gimple_vuse (stmt))
> + ? ? ? ? ?&& (flag_strict_enum_precision
> + ? ? ? ? ? ? ?|| TREE_CODE (TREE_TYPE (lhs)) != ENUMERAL_TYPE))
> ? ? ? ?return true;
> ? ? }
> ? else if (gimple_code (stmt) == GIMPLE_COND
>
>
> --
> This patch is available for review at http://codereview.appspot.com/4433083
>