diff mbox

Disable an unsafe VRP transformation when -fno-strict-overflow is set

Message ID 1416453660-19905-1-git-send-email-patrick@parcs.ath.cx
State New
Headers show

Commit Message

Patrick Palka Nov. 20, 2014, 3:21 a.m. UTC
VRP may simplify a conditional like i <= 5 to i == 5 if it is known that
the lower bound of i's range is 5, e.g. [5, +INF].  But if the upper
bound of i's range is also overflow infinity, i.e. [5, +INF(OVF)] then
this transformation is only valid if -fstrict-overflow is in effect.
Likewise for transforming i > 10 to i != 10 given i's range is
[10, +INF(OVF)] and for transforming i <= 20 to i == 20 given i's range
is [-INF(OVF), 20].

This patch makes this transformation only get performed if strict
overflow rules are in effect and potentially emits a -Wstrict-overflow=3
warning when the transformation takes place.

Bootstrap and regtesting in progress on x86_64-unknown-linux-gnu.  Does
the patch look OK if there are no new regressions?

gcc/
	* tree-vrp.c (test_for_singularity): New parameter
	strict_overflow_p.  Set *strict_overflow_p to true if signed
	overflow must be undefined for the return value to satisfy the
	conditional.
	(simplify_cond_using_ranges): Don't perform the simplification
	if it violates overflow rules.

gcc/testsuite/
	* gcc.dg/no-strict-overflow-8.c: New test.
---
 gcc/testsuite/gcc.dg/no-strict-overflow-8.c | 25 +++++++++++++
 gcc/tree-vrp.c                              | 57 +++++++++++++++++++++++++----
 2 files changed, 74 insertions(+), 8 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/no-strict-overflow-8.c

Comments

Patrick Palka Nov. 20, 2014, 3:28 a.m. UTC | #1
On Wed, Nov 19, 2014 at 10:21 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> VRP may simplify a conditional like i <= 5 to i == 5 if it is known that
> the lower bound of i's range is 5, e.g. [5, +INF].  But if the upper
> bound of i's range is also overflow infinity, i.e. [5, +INF(OVF)] then
> this transformation is only valid if -fstrict-overflow is in effect.
> Likewise for transforming i > 10 to i != 10 given i's range is
> [10, +INF(OVF)] and for transforming i <= 20 to i == 20 given i's range
> is [-INF(OVF), 20].

Oops, I meant to say ...and for transforming i >= 20 to i == 20 given
i's range is [-INF(OVF), 20].
Richard Biener Nov. 20, 2014, 1:17 p.m. UTC | #2
On Thu, Nov 20, 2014 at 4:21 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> VRP may simplify a conditional like i <= 5 to i == 5 if it is known that
> the lower bound of i's range is 5, e.g. [5, +INF].  But if the upper
> bound of i's range is also overflow infinity, i.e. [5, +INF(OVF)] then
> this transformation is only valid if -fstrict-overflow is in effect.
> Likewise for transforming i > 10 to i != 10 given i's range is
> [10, +INF(OVF)] and for transforming i <= 20 to i == 20 given i's range
> is [-INF(OVF), 20].
>
> This patch makes this transformation only get performed if strict
> overflow rules are in effect and potentially emits a -Wstrict-overflow=3
> warning when the transformation takes place.
>
> Bootstrap and regtesting in progress on x86_64-unknown-linux-gnu.  Does
> the patch look OK if there are no new regressions?

Ok.

Thanks,
Richard.

> gcc/
>         * tree-vrp.c (test_for_singularity): New parameter
>         strict_overflow_p.  Set *strict_overflow_p to true if signed
>         overflow must be undefined for the return value to satisfy the
>         conditional.
>         (simplify_cond_using_ranges): Don't perform the simplification
>         if it violates overflow rules.
>
> gcc/testsuite/
>         * gcc.dg/no-strict-overflow-8.c: New test.
> ---
>  gcc/testsuite/gcc.dg/no-strict-overflow-8.c | 25 +++++++++++++
>  gcc/tree-vrp.c                              | 57 +++++++++++++++++++++++++----
>  2 files changed, 74 insertions(+), 8 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/no-strict-overflow-8.c
>
> diff --git a/gcc/testsuite/gcc.dg/no-strict-overflow-8.c b/gcc/testsuite/gcc.dg/no-strict-overflow-8.c
> new file mode 100644
> index 0000000..11ef935
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/no-strict-overflow-8.c
> @@ -0,0 +1,25 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fno-strict-overflow -O2 -fdump-tree-optimized" } */
> +
> +/* We cannot fold i > 0 because p->a - p->b can be larger than INT_MAX
> +   and thus i can wrap.  Dual of Wstrict-overflow-18.c  */
> +
> +struct c { unsigned int a; unsigned int b; };
> +extern void bar (struct c *);
> +int
> +foo (struct c *p)
> +{
> +  int i;
> +  int sum = 0;
> +
> +  for (i = 0; i < p->a - p->b; ++i)
> +    {
> +      if (i > 0)
> +       sum += 2;
> +      bar (p);
> +    }
> +  return sum;
> +}
> +
> +/* { dg-final { scan-tree-dump "i_.* > 0" "optimized" } } */
> +/* { dg-final { cleanup-tree-dump "optimized" } } */
> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
> index bcf4c2b..444af71 100644
> --- a/gcc/tree-vrp.c
> +++ b/gcc/tree-vrp.c
> @@ -9117,11 +9117,15 @@ simplify_bit_ops_using_ranges (gimple_stmt_iterator *gsi, gimple stmt)
>     a known value range VR.
>
>     If there is one and only one value which will satisfy the
> -   conditional, then return that value.  Else return NULL.  */
> +   conditional, then return that value.  Else return NULL.
> +
> +   If signed overflow must be undefined for the value to satisfy
> +   the conditional, then set *STRICT_OVERFLOW_P to true.  */
>
>  static tree
>  test_for_singularity (enum tree_code cond_code, tree op0,
> -                     tree op1, value_range_t *vr)
> +                     tree op1, value_range_t *vr,
> +                     bool *strict_overflow_p)
>  {
>    tree min = NULL;
>    tree max = NULL;
> @@ -9172,7 +9176,16 @@ test_for_singularity (enum tree_code cond_code, tree op0,
>          then there is only one value which can satisfy the condition,
>          return that value.  */
>        if (operand_equal_p (min, max, 0) && is_gimple_min_invariant (min))
> -       return min;
> +       {
> +         if ((cond_code == LE_EXPR || cond_code == LT_EXPR)
> +             && is_overflow_infinity (vr->max))
> +           *strict_overflow_p = true;
> +         if ((cond_code == GE_EXPR || cond_code == GT_EXPR)
> +             && is_overflow_infinity (vr->min))
> +           *strict_overflow_p = true;
> +
> +         return min;
> +       }
>      }
>    return NULL;
>  }
> @@ -9252,9 +9265,12 @@ simplify_cond_using_ranges (gcond *stmt)
>          able to simplify this conditional. */
>        if (vr->type == VR_RANGE)
>         {
> -         tree new_tree = test_for_singularity (cond_code, op0, op1, vr);
> +         enum warn_strict_overflow_code wc = WARN_STRICT_OVERFLOW_CONDITIONAL;
> +         bool sop = false;
> +         tree new_tree = test_for_singularity (cond_code, op0, op1, vr, &sop);
>
> -         if (new_tree)
> +         if (new_tree
> +             && (!sop || TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (op0))))
>             {
>               if (dump_file)
>                 {
> @@ -9275,16 +9291,30 @@ simplify_cond_using_ranges (gcond *stmt)
>                   fprintf (dump_file, "\n");
>                 }
>
> +             if (sop && issue_strict_overflow_warning (wc))
> +               {
> +                 location_t location = input_location;
> +                 if (gimple_has_location (stmt))
> +                   location = gimple_location (stmt);
> +
> +                 warning_at (location, OPT_Wstrict_overflow,
> +                             "assuming signed overflow does not occur when "
> +                             "simplifying conditional");
> +               }
> +
>               return true;
>             }
>
>           /* Try again after inverting the condition.  We only deal
>              with integral types here, so no need to worry about
>              issues with inverting FP comparisons.  */
> -         cond_code = invert_tree_comparison (cond_code, false);
> -         new_tree = test_for_singularity (cond_code, op0, op1, vr);
> +         sop = false;
> +         new_tree = test_for_singularity
> +                      (invert_tree_comparison (cond_code, false),
> +                       op0, op1, vr, &sop);
>
> -         if (new_tree)
> +         if (new_tree
> +             && (!sop || TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (op0))))
>             {
>               if (dump_file)
>                 {
> @@ -9305,6 +9335,17 @@ simplify_cond_using_ranges (gcond *stmt)
>                   fprintf (dump_file, "\n");
>                 }
>
> +             if (sop && issue_strict_overflow_warning (wc))
> +               {
> +                 location_t location = input_location;
> +                 if (gimple_has_location (stmt))
> +                   location = gimple_location (stmt);
> +
> +                 warning_at (location, OPT_Wstrict_overflow,
> +                             "assuming signed overflow does not occur when "
> +                             "simplifying conditional");
> +               }
> +
>               return true;
>             }
>         }
> --
> 2.2.0.rc1.23.gf570943
>
diff mbox

Patch

diff --git a/gcc/testsuite/gcc.dg/no-strict-overflow-8.c b/gcc/testsuite/gcc.dg/no-strict-overflow-8.c
new file mode 100644
index 0000000..11ef935
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/no-strict-overflow-8.c
@@ -0,0 +1,25 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fno-strict-overflow -O2 -fdump-tree-optimized" } */
+
+/* We cannot fold i > 0 because p->a - p->b can be larger than INT_MAX
+   and thus i can wrap.  Dual of Wstrict-overflow-18.c  */
+
+struct c { unsigned int a; unsigned int b; };
+extern void bar (struct c *);
+int
+foo (struct c *p)
+{
+  int i;
+  int sum = 0;
+
+  for (i = 0; i < p->a - p->b; ++i)
+    {
+      if (i > 0)
+	sum += 2;
+      bar (p);
+    }
+  return sum;
+}
+
+/* { dg-final { scan-tree-dump "i_.* > 0" "optimized" } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index bcf4c2b..444af71 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -9117,11 +9117,15 @@  simplify_bit_ops_using_ranges (gimple_stmt_iterator *gsi, gimple stmt)
    a known value range VR.
 
    If there is one and only one value which will satisfy the
-   conditional, then return that value.  Else return NULL.  */
+   conditional, then return that value.  Else return NULL.
+
+   If signed overflow must be undefined for the value to satisfy
+   the conditional, then set *STRICT_OVERFLOW_P to true.  */
 
 static tree
 test_for_singularity (enum tree_code cond_code, tree op0,
-		      tree op1, value_range_t *vr)
+		      tree op1, value_range_t *vr,
+		      bool *strict_overflow_p)
 {
   tree min = NULL;
   tree max = NULL;
@@ -9172,7 +9176,16 @@  test_for_singularity (enum tree_code cond_code, tree op0,
 	 then there is only one value which can satisfy the condition,
 	 return that value.  */
       if (operand_equal_p (min, max, 0) && is_gimple_min_invariant (min))
-	return min;
+	{
+	  if ((cond_code == LE_EXPR || cond_code == LT_EXPR)
+	      && is_overflow_infinity (vr->max))
+	    *strict_overflow_p = true;
+	  if ((cond_code == GE_EXPR || cond_code == GT_EXPR)
+	      && is_overflow_infinity (vr->min))
+	    *strict_overflow_p = true;
+
+	  return min;
+	}
     }
   return NULL;
 }
@@ -9252,9 +9265,12 @@  simplify_cond_using_ranges (gcond *stmt)
 	 able to simplify this conditional. */
       if (vr->type == VR_RANGE)
 	{
-	  tree new_tree = test_for_singularity (cond_code, op0, op1, vr);
+	  enum warn_strict_overflow_code wc = WARN_STRICT_OVERFLOW_CONDITIONAL;
+	  bool sop = false;
+	  tree new_tree = test_for_singularity (cond_code, op0, op1, vr, &sop);
 
-	  if (new_tree)
+	  if (new_tree
+	      && (!sop || TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (op0))))
 	    {
 	      if (dump_file)
 		{
@@ -9275,16 +9291,30 @@  simplify_cond_using_ranges (gcond *stmt)
 		  fprintf (dump_file, "\n");
 		}
 
+	      if (sop && issue_strict_overflow_warning (wc))
+	        {
+	          location_t location = input_location;
+	          if (gimple_has_location (stmt))
+		    location = gimple_location (stmt);
+
+	          warning_at (location, OPT_Wstrict_overflow,
+			      "assuming signed overflow does not occur when "
+			      "simplifying conditional");
+	        }
+
 	      return true;
 	    }
 
 	  /* Try again after inverting the condition.  We only deal
 	     with integral types here, so no need to worry about
 	     issues with inverting FP comparisons.  */
-	  cond_code = invert_tree_comparison (cond_code, false);
-	  new_tree = test_for_singularity (cond_code, op0, op1, vr);
+	  sop = false;
+	  new_tree = test_for_singularity
+		       (invert_tree_comparison (cond_code, false),
+			op0, op1, vr, &sop);
 
-	  if (new_tree)
+	  if (new_tree
+	      && (!sop || TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (op0))))
 	    {
 	      if (dump_file)
 		{
@@ -9305,6 +9335,17 @@  simplify_cond_using_ranges (gcond *stmt)
 		  fprintf (dump_file, "\n");
 		}
 
+	      if (sop && issue_strict_overflow_warning (wc))
+	        {
+	          location_t location = input_location;
+	          if (gimple_has_location (stmt))
+		    location = gimple_location (stmt);
+
+	          warning_at (location, OPT_Wstrict_overflow,
+			      "assuming signed overflow does not occur when "
+			      "simplifying conditional");
+	        }
+
 	      return true;
 	    }
 	}