This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

PR 61136: wide-int fallout in int_const_binop_1

The wide-int version of int_const_binop_1 has:

static tree
int_const_binop_1 (enum tree_code code, const_tree arg1, const_tree parg2,
		   int overflowable)
  tree type = TREE_TYPE (arg1);
  wide_int arg2 = wide_int::from (parg2, TYPE_PRECISION (type),
				  TYPE_SIGN (TREE_TYPE (parg2)));

The idea that the result should be the same type as arg1 predates
wide-int but the conversion of arg2 to that type is new.

The new conversion can't be right because it loses overflow information.
The question then is whether this function should be prepared to handle
mismatched arguments (as the pre-wide-int version effectively did, since
it did everything in double_int precision) or whether the operation
should be strictly typed.  If the former then the function should be
doing the calculation in widest_int, which would make it a much more
direct analogue of the original code.  If the latter then we should be
able to operate directly on arg2.

I assume it really is supposed to be strictly typed though.  It seems
strange to use the type of the first operand as the type of the result
otherwise.  E.g. in one of the cases I've found where it isn't strictly
typed we create:

 <mult_expr 0x7ffff1310758
    type <integer_type 0x7ffff11ec888 long unsigned int public unsigned DI
        size <integer_cst 0x7ffff11daf78 constant 64>
        unit size <integer_cst 0x7ffff11daf90 constant 8>
        align 64 symtab 0 alias set -1 canonical type 0x7ffff11ec888 precision 64 min <integer_cst 0x7ffff11f9258 0> max <integer_cst 0x7ffff11f0100 18446744073709551615>>

    arg 0 <integer_cst 0x7ffff11f9720 type <integer_type 0x7ffff11ec690 int> constant 4>
    arg 1 <sizeof_expr 0x7ffff130d280 type <integer_type 0x7ffff11ec888 long unsigned int>
        arg 0 <integer_type 0x7ffff11ec690 int public type_6 SI
            size <integer_cst 0x7ffff11f91c8 constant 32>
            unit size <integer_cst 0x7ffff11f91e0 constant 4>
            align 32 symtab 0 alias set -1 canonical type 0x7ffff11ec690 precision 32 min <integer_cst 0x7ffff11f9180 -2147483648> max <integer_cst 0x7ffff11f9198 2147483647>
            pointer_to_this <pointer_type 0x7ffff1200738>>

and these arguments later find their way to int_const_binop_1.
The result and second argument are long (64 bits) but the first
argument is int (32 bits).  So a simple conversion to widest_int
would mean that the result would be 32 bits rather than 64 bits,
which doesn't look intentional.

I tried making int_const_binop_1 strictly typed and the only thing
that was needed to pass bootstrap was a fairly obvious-looking patch
to the Ada frontend.  But there's quite a bit of fallout in the testsuite
itself (including the above).  I'm still working my way through that,
so please shout if I'm going doing a rathole.

This causes a problem in the testcase because of this code in

    case INTEGER_CST:
      if (TREE_CODE (bottom) != INTEGER_CST
	  || integer_zerop (bottom)
	  || (TYPE_UNSIGNED (type)
	      && (tree_int_cst_sgn (top) < 0
		  || tree_int_cst_sgn (bottom) < 0)))
	return 0;
      return integer_zerop (int_const_binop (TRUNC_MOD_EXPR,
					     top, bottom));

Here TOP and BOTTOM aren't necessarily the same type.  In the testcase
for PR61136 BOTTOM is wider than TOP and when converted (truncated)
to TOP's type becomes zero.  This then falls foul of the null escape in:

    case TRUNC_MOD_EXPR:
      if (arg2 == 0)
	return NULL_TREE;
      res = wi::mod_trunc (arg1, arg2, sign, &overflow);

So we end up calling integer_zerop on a null tree.

We know at the point of the int_const_binop call that we're dealing
with two INTEGER_CSTs, so it seems silly to create a tree for the result
only to test whether it's zero.  This patch therefore uses wi:: directly.
I think this is worth doing independently of the eventual patches to fix
the type incompatibilities elsewhere.

The test is "wi::smod_trunc (...) == 0" but it seemed more mnemonic to
have a version of wi::multiple_of_p that doesn't compute the quotient.

Tested on x86_64-linux-gnu.  OK to install?


	PR tree-optimization/61136
	* wide-int.h (multiple_of_p): Define a version that doesn't return
	the quotient.
	* fold-const.c (extract_muldiv_1): Use wi::multiple_of_p instead of an
	integer_zerop/const_binop pair.
	(multiple_of_p): Likewise, converting both operands to widest_int

	* gcc.dg/torture/pr61136.c: New test.

Index: gcc/wide-int.h
--- gcc/wide-int.h	2014-05-10 19:45:34.743137375 +0100
+++ gcc/wide-int.h	2014-05-10 20:03:17.275435486 +0100
@@ -530,6 +530,9 @@ #define SHIFT_FUNCTION \
   BINARY_FUNCTION mod_round (const T1 &, const T2 &, signop, bool * = 0);
   template <typename T1, typename T2>
+  bool multiple_of_p (const T1 &, const T2 &, signop);
+  template <typename T1, typename T2>
   bool multiple_of_p (const T1 &, const T2 &, signop,
 		      WI_BINARY_RESULT (T1, T2) *);
@@ -2791,6 +2794,15 @@ wi::mod_round (const T1 &x, const T2 &y,
   return remainder;
+/* Return true if X is a multiple of Y.  Treat X and Y as having the
+   signedness given by SGN.  */
+template <typename T1, typename T2>
+inline bool
+wi::multiple_of_p (const T1 &x, const T2 &y, signop sgn)
+  return wi::mod_trunc (x, y, sgn) == 0;
 /* Return true if X is a multiple of Y, storing X / Y in *RES if so.
    Treat X and Y as having the signedness given by SGN.  */
 template <typename T1, typename T2>
Index: gcc/fold-const.c
--- gcc/fold-const.c	2014-05-10 19:45:34.743137375 +0100
+++ gcc/fold-const.c	2014-05-10 20:04:55.661262761 +0100
@@ -5708,7 +5708,7 @@ extract_muldiv_1 (tree t, tree c, enum t
       /* For a constant, we can always simplify if we are a multiply
 	 or (for divide and modulus) if it is a multiple of our constant.  */
       if (code == MULT_EXPR
-	  || integer_zerop (const_binop (TRUNC_MOD_EXPR, t, c)))
+	  || wi::multiple_of_p (t, c, TYPE_SIGN (type)))
 	return const_binop (code, fold_convert (ctype, t),
 			    fold_convert (ctype, c));
@@ -5888,7 +5888,7 @@ extract_muldiv_1 (tree t, tree c, enum t
       /* If it's a multiply or a division/modulus operation of a multiple
          of our constant, do the operation and verify it doesn't overflow.  */
       if (code == MULT_EXPR
-	  || integer_zerop (const_binop (TRUNC_MOD_EXPR, op1, c)))
+	  || wi::multiple_of_p (op1, c, TYPE_SIGN (type)))
 	  op1 = const_binop (code, fold_convert (ctype, op1),
 			     fold_convert (ctype, c));
@@ -5932,7 +5932,7 @@ extract_muldiv_1 (tree t, tree c, enum t
 	  /* If the multiplication can overflow we cannot optimize this.  */
-	  && integer_zerop (const_binop (TRUNC_MOD_EXPR, op1, c)))
+	  && wi::multiple_of_p (op1, c, TYPE_SIGN (type)))
 	  *strict_overflow_p = true;
 	  return omit_one_operand (type, integer_zero_node, op0);
@@ -5989,7 +5989,7 @@ extract_muldiv_1 (tree t, tree c, enum t
 		  && code != FLOOR_MOD_EXPR && code != ROUND_MOD_EXPR
 		  && code != MULT_EXPR)))
-	  if (integer_zerop (const_binop (TRUNC_MOD_EXPR, op1, c)))
+	  if (wi::multiple_of_p (op1, c, TYPE_SIGN (type)))
 	      if (TYPE_OVERFLOW_UNDEFINED (ctype))
 		*strict_overflow_p = true;
@@ -5998,7 +5998,7 @@ extract_muldiv_1 (tree t, tree c, enum t
 						const_binop (TRUNC_DIV_EXPR,
 							     op1, c)));
-	  else if (integer_zerop (const_binop (TRUNC_MOD_EXPR, c, op1)))
+	  else if (wi::multiple_of_p (c, op1, TYPE_SIGN (type)))
 	      if (TYPE_OVERFLOW_UNDEFINED (ctype))
 		*strict_overflow_p = true;
@@ -15314,8 +15314,8 @@ multiple_of_p (tree type, const_tree top
 	      && (tree_int_cst_sgn (top) < 0
 		  || tree_int_cst_sgn (bottom) < 0)))
 	return 0;
-      return integer_zerop (int_const_binop (TRUNC_MOD_EXPR,
-					     top, bottom));
+      return wi::multiple_of_p (wi::to_widest (top), wi::to_widest (bottom),
+				SIGNED);
       return 0;
Index: gcc/testsuite/gcc.dg/torture/pr61136.c
--- /dev/null	2014-05-03 11:58:38.033951363 +0100
+++ gcc/testsuite/gcc.dg/torture/pr61136.c	2014-05-10 20:03:17.274435478 +0100
@@ -0,0 +1,5 @@
+unsigned long long
+foo (int a)
+  return a * 7 & 1ULL << 63;

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]