diff mbox

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

Message ID 20110428195045.E4A35AE1D5@tobiano.tor.corp.google.com
State New
Headers show

Commit Message

Diego Novillo April 28, 2011, 7:50 p.m. UTC 
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?

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.



--
This patch is available for review at http://codereview.appspot.com/4433083

Comments

Nathan Froyd April 28, 2011, 7:56 p.m. UTC | #1 
On Thu, Apr 28, 2011 at 03:50:45PM -0400, Diego Novillo wrote:
> Committed to google/main.  Jason, Silvius, what do you think would be
> the best approach to merge this into trunk?

When this code does get merged to trunk, can the testcases abort() on
failure rather than returning 1?  This is friendlier for embedded target
testing.

-Nathan
Paolo Carlini April 28, 2011, 7:57 p.m. UTC | #2 
... are the testcases formatted according to the GNU guidelines, in terms, for example, of open and closed curly braces? I don't think so, I see some weird (sorry, after all those years unavoidably look to me *really* weird) open braces ending lines?

Paolo
Diego Novillo April 28, 2011, 8:23 p.m. UTC | #3 
On Thu, Apr 28, 2011 at 15:56, Nathan Froyd <froydnj@codesourcery.com> wrote:
> On Thu, Apr 28, 2011 at 03:50:45PM -0400, Diego Novillo wrote:
>> Committed to google/main.  Jason, Silvius, what do you think would be
>> the best approach to merge this into trunk?
>
> When this code does get merged to trunk, can the testcases abort() on
> failure rather than returning 1?  This is friendlier for embedded target
> testing.

Sure.  I've changed all the returns to abort() or exit(0).


Diego.
Diego Novillo April 28, 2011, 8:24 p.m. UTC | #4 
On Thu, Apr 28, 2011 at 15:57, Paolo Carlini <pcarlini@gmail.com> wrote:
> ... are the testcases formatted according to the GNU guidelines

They weren't.  I've run indent -gnu on all of them.  Thanks for noticing.


Diego.
Richard Biener April 28, 2011, 11:25 p.m. UTC | #5 
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
>
Jason Merrill May 3, 2011, 6:34 p.m. UTC | #6 
On 04/28/2011 03:50 PM, Diego Novillo 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?
>
> We use it internally to disable this optimization for code that
> generates values outside of enum ranges.

It seems that to me that this patch changes optimizations to not believe 
the lies of the C++ front end, whereas my patch changes the front end to 
not lie in the first place, making this patch unnecessary.

Jason
Richard Biener May 4, 2011, 9:02 a.m. UTC | #7 
On Tue, May 3, 2011 at 8:34 PM, Jason Merrill <jason@redhat.com> wrote:
> On 04/28/2011 03:50 PM, Diego Novillo 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?
>>
>> We use it internally to disable this optimization for code that
>> generates values outside of enum ranges.
>
> It seems that to me that this patch changes optimizations to not believe the
> lies of the C++ front end, whereas my patch changes the front end to not lie
> in the first place, making this patch unnecessary.

Correct.

Richard.
diff mbox

Patch

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