diff mbox series

fold-const: Fix up make_range_step [PR105189]

Message ID Yk8Hb8PkPLT3a6As@tucnak
State New
Headers show
Series fold-const: Fix up make_range_step [PR105189] | expand

Commit Message

Jakub Jelinek April 7, 2022, 3:46 p.m. UTC
Hi!

The following testcase is miscompiled, because fold_truth_andor
incorrectly folds
(unsigned) foo () >= 0U && 1
into
foo () >= 0
For the unsigned comparison (which is useless in this case,
as >= 0U is always true, but hasn't been folded yet), previous
make_range_step derives exp (unsigned) foo () and
+[0U, -]
range for it.  Next we process the NOP_EXPR.  We have special code
for unsigned to signed casts, already earlier punt if low or high
aren't representable in arg0_type or if it is a narrowing conversion.
For the signed to unsigned casts, I think if high is specified we
are still fine, as we punt for non-representable values in arg0_type,
n_high is then still representable and so was smaller or equal to
signed maximum and either low is not present (equivalent to 0U), or
low must be smaller or equal to high and so for unsigned exp
+[low, high] the signed exp +[n_low, n_high] will be correct.
Similarly, if both low and high aren't specified (always true or
always false), it is ok too.
But if we have for unsigned exp +[low, -] or -[low, -], using
+[n_low, -] or -[n_high, -] is incorrect.  Because low is smaller
or equal to signed maximum and high is unspecified (i.e. unsigned
maximum), when signed that range is a union of +[n_low, -] and
+[-, -1] which is equivalent to -[0, n_low-1], unless low
is 0, in that case we can treat it as [-, -].

Bootstrapped/regtested on powerpc64le-linux, ok for trunk?

2022-04-07  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/105189
	* fold-const.cc (make_range_step): Fix up handling of
	(unsigned) x +[low, -] ranges for signed x if low fits into
	typeof (x).

	* g++.dg/torture/pr105189.C: New test.


	Jakub

Comments

Richard Biener April 7, 2022, 4:15 p.m. UTC | #1
> Am 07.04.2022 um 17:47 schrieb Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org>:
> 
> Hi!
> 
> The following testcase is miscompiled, because fold_truth_andor
> incorrectly folds
> (unsigned) foo () >= 0U && 1
> into
> foo () >= 0
> For the unsigned comparison (which is useless in this case,
> as >= 0U is always true, but hasn't been folded yet), previous
> make_range_step derives exp (unsigned) foo () and
> +[0U, -]
> range for it.  Next we process the NOP_EXPR.  We have special code
> for unsigned to signed casts, already earlier punt if low or high
> aren't representable in arg0_type or if it is a narrowing conversion.
> For the signed to unsigned casts, I think if high is specified we
> are still fine, as we punt for non-representable values in arg0_type,
> n_high is then still representable and so was smaller or equal to
> signed maximum and either low is not present (equivalent to 0U), or
> low must be smaller or equal to high and so for unsigned exp
> +[low, high] the signed exp +[n_low, n_high] will be correct.
> Similarly, if both low and high aren't specified (always true or
> always false), it is ok too.
> But if we have for unsigned exp +[low, -] or -[low, -], using
> +[n_low, -] or -[n_high, -] is incorrect.  Because low is smaller
> or equal to signed maximum and high is unspecified (i.e. unsigned
> maximum), when signed that range is a union of +[n_low, -] and
> +[-, -1] which is equivalent to -[0, n_low-1], unless low
> is 0, in that case we can treat it as [-, -].
> 
> Bootstrapped/regtested on powerpc64le-linux, ok for trunk?

Ok.

Thanks,
Richard 

> 2022-04-07  Jakub Jelinek  <jakub@redhat.com>
> 
>    PR tree-optimization/105189
>    * fold-const.cc (make_range_step): Fix up handling of
>    (unsigned) x +[low, -] ranges for signed x if low fits into
>    typeof (x).
> 
>    * g++.dg/torture/pr105189.C: New test.
> 
> --- gcc/fold-const.cc.jj    2022-04-04 12:09:00.317116906 +0200
> +++ gcc/fold-const.cc    2022-04-07 11:32:41.185313269 +0200
> @@ -5212,7 +5212,7 @@ make_range_step (location_t loc, enum tr
>    n_high = fold_convert_loc (loc, arg0_type, n_high);
> 
>       /* If we're converting arg0 from an unsigned type, to exp,
> -     a signed type,  we will be doing the comparison as unsigned.
> +     a signed type, we will be doing the comparison as unsigned.
>     The tests above have already verified that LOW and HIGH
>     are both positive.
> 
> @@ -5274,6 +5274,32 @@ make_range_step (location_t loc, enum tr
>        }
>    }
> 
> +      /* Otherwise, if we are converting arg0 from signed type, to exp,
> +     an unsigned type, we will do the comparison as signed.  If
> +     high is non-NULL, we punt above if it doesn't fit in the signed
> +     type, so if we get through here, +[-, high] or +[low, high] are
> +     equivalent to +[-, n_high] or +[n_low, n_high].  Similarly,
> +     +[-, -] or -[-, -] are equivalent too.  But if low is specified and
> +     high is not, the +[low, -] range is equivalent to union of
> +     +[n_low, -] and +[-, -1] ranges, so +[low, -] is equivalent to
> +     -[0, n_low-1] and similarly -[low, -] to +[0, n_low-1], except for
> +     low being 0, which should be treated as [-, -].  */
> +      else if (TYPE_UNSIGNED (exp_type)
> +           && !TYPE_UNSIGNED (arg0_type)
> +           && low
> +           && !high)
> +    {
> +      if (integer_zerop (low))
> +        n_low = NULL_TREE;
> +      else
> +        {
> +          n_high = fold_build2_loc (loc, PLUS_EXPR, arg0_type,
> +                    n_low, build_int_cst (arg0_type, -1));
> +          n_low = build_zero_cst (arg0_type);
> +          in_p = !in_p;
> +        }
> +    }
> +
>       *p_low = n_low;
>       *p_high = n_high;
>       *p_in_p = in_p;
> --- gcc/testsuite/g++.dg/torture/pr105189.C.jj    2022-04-07 11:40:06.195098778 +0200
> +++ gcc/testsuite/g++.dg/torture/pr105189.C    2022-04-07 11:39:50.177322461 +0200
> @@ -0,0 +1,19 @@
> +// PR tree-optimization/105189
> +// { dg-do run }
> +
> +int
> +foo ()
> +{
> +  return -1;
> +}
> +
> +int
> +main ()
> +{
> +  int c = foo () >= 0U && 1;
> +  if (c != 1)
> +    __builtin_abort ();
> +  int d = foo () >= 3U && 1;
> +  if (d != 1)
> +    __builtin_abort ();
> +}
> 
>    Jakub
>
diff mbox series

Patch

--- gcc/fold-const.cc.jj	2022-04-04 12:09:00.317116906 +0200
+++ gcc/fold-const.cc	2022-04-07 11:32:41.185313269 +0200
@@ -5212,7 +5212,7 @@  make_range_step (location_t loc, enum tr
 	n_high = fold_convert_loc (loc, arg0_type, n_high);
 
       /* If we're converting arg0 from an unsigned type, to exp,
-	 a signed type,  we will be doing the comparison as unsigned.
+	 a signed type, we will be doing the comparison as unsigned.
 	 The tests above have already verified that LOW and HIGH
 	 are both positive.
 
@@ -5274,6 +5274,32 @@  make_range_step (location_t loc, enum tr
 	    }
 	}
 
+      /* Otherwise, if we are converting arg0 from signed type, to exp,
+	 an unsigned type, we will do the comparison as signed.  If
+	 high is non-NULL, we punt above if it doesn't fit in the signed
+	 type, so if we get through here, +[-, high] or +[low, high] are
+	 equivalent to +[-, n_high] or +[n_low, n_high].  Similarly,
+	 +[-, -] or -[-, -] are equivalent too.  But if low is specified and
+	 high is not, the +[low, -] range is equivalent to union of
+	 +[n_low, -] and +[-, -1] ranges, so +[low, -] is equivalent to
+	 -[0, n_low-1] and similarly -[low, -] to +[0, n_low-1], except for
+	 low being 0, which should be treated as [-, -].  */
+      else if (TYPE_UNSIGNED (exp_type)
+	       && !TYPE_UNSIGNED (arg0_type)
+	       && low
+	       && !high)
+	{
+	  if (integer_zerop (low))
+	    n_low = NULL_TREE;
+	  else
+	    {
+	      n_high = fold_build2_loc (loc, PLUS_EXPR, arg0_type,
+					n_low, build_int_cst (arg0_type, -1));
+	      n_low = build_zero_cst (arg0_type);
+	      in_p = !in_p;
+	    }
+	}
+
       *p_low = n_low;
       *p_high = n_high;
       *p_in_p = in_p;
--- gcc/testsuite/g++.dg/torture/pr105189.C.jj	2022-04-07 11:40:06.195098778 +0200
+++ gcc/testsuite/g++.dg/torture/pr105189.C	2022-04-07 11:39:50.177322461 +0200
@@ -0,0 +1,19 @@ 
+// PR tree-optimization/105189
+// { dg-do run }
+
+int
+foo ()
+{
+  return -1;
+}
+
+int
+main ()
+{
+  int c = foo () >= 0U && 1;
+  if (c != 1)
+    __builtin_abort ();
+  int d = foo () >= 3U && 1;
+  if (d != 1)
+    __builtin_abort ();
+}