[RFC] PR34389 -Wconversion produces wrong warning

[RFC] PR34389 -Wconversion produces wrong warning

[Manuel López-Ibáñez]

[-- Attachment #1: Type: text/plain, Size: 917 bytes --]

The following patch  fixes PR 34389 in a ugly way, in my own opinion,
but I am not able to come up with any other way to fix this.

A more detailed analysis is in the PR. In summary the problem here is
that fold_unary converts (T)(x & c) into (T)x & (T)c which in this
case is (int) (x & 0x7fff) into ((int)x & 0x7fff). By using
get_unwidened, we could handle the first form. However, get_unwidened
won't handle the latter. Thus, when Wconversion sees that this
expression is assigned to a short, it concludes that there may be a
loss of precision.

Bootstrapped and regression tested in x86_64-unknown-linux-gnu. This
patch only affects Wconversion, which is new in 4.3.

Is this OK to commit? Any suggestions?

Cheers,

Manuel.

2008-02-16  Manuel Lopez-Ibanez  <manu@gcc.gnu.org>

 PR c/34389
 * c-common.c (conversion_warning): Handle '&' expressions correctly.

testsuite/
  * gcc.dg/Wconversion-pr34389.c: New.

[-- Attachment #2: fix-pr34389.diff --]
[-- Type: text/plain, Size: 5336 bytes --]

Index: gcc/testsuite/gcc.dg/Wconversion-pr34389.c
===================================================================
--- gcc/testsuite/gcc.dg/Wconversion-pr34389.c	(revision 0)
+++ gcc/testsuite/gcc.dg/Wconversion-pr34389.c	(revision 0)
@@ -0,0 +1,13 @@
+/* PR 34389 */
+/* { dg-do compile } */
+/* { dg-options "-Wall -Wextra -Wconversion -Wsign-conversion" } */
+short  mask1(short x)
+{
+  short y = 0x7fff;
+  return x & y;
+}
+
+short  mask2(short x)
+{
+  return x & 0x7fff;
+}
Index: gcc/c-common.c
===================================================================
--- gcc/c-common.c	(revision 132310)
+++ gcc/c-common.c	(working copy)
@@ -1270,65 +1270,98 @@ conversion_warning (tree type, tree expr
                  "conversion to %qT alters %qT constant value",
                  type, TREE_TYPE (expr));
     }
   else /* 'expr' is not a constant.  */
     {
+      tree expr_type = TREE_TYPE (expr);
+
       /* Warn for real types converted to integer types.  */
-      if (TREE_CODE (TREE_TYPE (expr)) == REAL_TYPE
+      if (TREE_CODE (expr_type) == REAL_TYPE
           && TREE_CODE (type) == INTEGER_TYPE)
         give_warning = true;
 
-      else if (TREE_CODE (TREE_TYPE (expr)) == INTEGER_TYPE
+      else if (TREE_CODE (expr_type) == INTEGER_TYPE
                && TREE_CODE (type) == INTEGER_TYPE)
         {
 	  /* Don't warn about unsigned char y = 0xff, x = (int) y;  */
 	  expr = get_unwidened (expr, 0);
+	  expr_type = TREE_TYPE (expr);
 
+	  /* Don't warn for short y; short x = ((int)y & 0xff);  */
+	  if (TREE_CODE (expr) == BIT_AND_EXPR
+	      && TREE_CODE (TREE_OPERAND (expr, 1)) == INTEGER_CST)
+	    {
+	      int unsigned0, unsigned1;
+	      tree arg0, arg1;
+	      tree tmp_type;
+	      tree op0, op1;
+
+	      op0 = convert (expr_type, TREE_OPERAND (expr, 0));
+	      op1 = convert (expr_type, TREE_OPERAND (expr, 1));
+
+	      arg0 = get_narrower (op0, &unsigned0);
+	      arg1 = get_narrower (op1, &unsigned1);
+
+	      /* Handle the case that OP0 (or OP1) does not *contain* a conversion
+		 but it *requires* conversion to TYPE.  */
+	      if ((TYPE_PRECISION (TREE_TYPE (op0))
+		   == TYPE_PRECISION (TREE_TYPE (arg0)))
+		  && TREE_TYPE (op0) != expr_type)
+		unsigned0 = TYPE_UNSIGNED (TREE_TYPE (op0));
+	      
+	      if (TYPE_PRECISION (TREE_TYPE (arg0)) < TYPE_PRECISION (expr_type)
+		  && (tmp_type
+		      = c_common_signed_or_unsigned_type (unsigned0,
+							   TREE_TYPE (arg0)))
+		  && !POINTER_TYPE_P (tmp_type)
+		  && int_fits_type_p (arg1, tmp_type))
+		expr_type = tmp_type;
+	    }
           /* Warn for integer types converted to smaller integer types.  */
-          if (formal_prec < TYPE_PRECISION (TREE_TYPE (expr))) 
+          if (formal_prec < TYPE_PRECISION (expr_type)) 
 	    give_warning = true;
 
 	  /* When they are the same width but different signedness,
 	     then the value may change.  */
-	  else if ((formal_prec == TYPE_PRECISION (TREE_TYPE (expr))
-		    && TYPE_UNSIGNED (TREE_TYPE (expr)) != TYPE_UNSIGNED (type))
+	  else if ((formal_prec == TYPE_PRECISION (expr_type)
+		    && TYPE_UNSIGNED (expr_type) != TYPE_UNSIGNED (type))
 		   /* Even when converted to a bigger type, if the type is
 		      unsigned but expr is signed, then negative values
 		      will be changed.  */
-		   || (TYPE_UNSIGNED (type) && !TYPE_UNSIGNED (TREE_TYPE (expr))))
+		   || (TYPE_UNSIGNED (type) && !TYPE_UNSIGNED (expr_type)))
 	    warning (OPT_Wsign_conversion,
 		     "conversion to %qT from %qT may change the sign of the result",
-		     type, TREE_TYPE (expr));
+		     type, expr_type);
         }
 
       /* Warn for integer types converted to real types if and only if
          all the range of values of the integer type cannot be
          represented by the real type.  */
-      else if (TREE_CODE (TREE_TYPE (expr)) == INTEGER_TYPE
+      else if (TREE_CODE (expr_type) == INTEGER_TYPE
                && TREE_CODE (type) == REAL_TYPE)
         {
-          tree type_low_bound = TYPE_MIN_VALUE (TREE_TYPE (expr));
-          tree type_high_bound = TYPE_MAX_VALUE (TREE_TYPE (expr));
+          tree type_low_bound = TYPE_MIN_VALUE (expr_type);
+          tree type_high_bound = TYPE_MAX_VALUE (expr_type);
           REAL_VALUE_TYPE real_low_bound = real_value_from_int_cst (0, type_low_bound);
           REAL_VALUE_TYPE real_high_bound = real_value_from_int_cst (0, type_high_bound);
 
           if (!exact_real_truncate (TYPE_MODE (type), &real_low_bound)
               || !exact_real_truncate (TYPE_MODE (type), &real_high_bound))
             give_warning = true;
         }
 
       /* Warn for real types converted to smaller real types.  */
-      else if (TREE_CODE (TREE_TYPE (expr)) == REAL_TYPE
+      else if (TREE_CODE (expr_type) == REAL_TYPE
                && TREE_CODE (type) == REAL_TYPE
-               && formal_prec < TYPE_PRECISION (TREE_TYPE (expr)))
+               && formal_prec < TYPE_PRECISION (expr_type))
         give_warning = true;
 
 
       if (give_warning)
         warning (OPT_Wconversion,
                  "conversion to %qT from %qT may alter its value",
-                 type, TREE_TYPE (expr));
+                 type, expr_type);
     }
 }
 
 /* Produce warnings after a conversion. RESULT is the result of
    converting EXPR to TYPE.  This is a helper function for

Re: [RFC] PR34389 -Wconversion produces wrong warning

[Richard Guenther]

On Feb 16, 2008 5:52 PM, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
> The following patch  fixes PR 34389 in a ugly way, in my own opinion,
> but I am not able to come up with any other way to fix this.
>
> A more detailed analysis is in the PR. In summary the problem here is
> that fold_unary converts (T)(x & c) into (T)x & (T)c which in this
> case is (int) (x & 0x7fff) into ((int)x & 0x7fff). By using
> get_unwidened, we could handle the first form. However, get_unwidened
> won't handle the latter. Thus, when Wconversion sees that this
> expression is assigned to a short, it concludes that there may be a
> loss of precision.
>
> Bootstrapped and regression tested in x86_64-unknown-linux-gnu. This
> patch only affects Wconversion, which is new in 4.3.

The correct fix is to not fold expressions in the C frontend before doing
these kinds of warnings.

Richard.

Re: [RFC] PR34389 -Wconversion produces wrong warning

[Manuel López-Ibáñez]

On 16/02/2008, Richard Guenther <richard.guenther@gmail.com> wrote:
>
> The correct fix is to not fold expressions in the C frontend before doing
> these kinds of warnings.
>

The fold is done by the last call to "convert" in build_binary_op in
c-typeck.c And this is not just the C front end but also C++.

So I don't see how folding can be prevented here.

Cheers,

Manuel.

Re: [RFC] PR34389 -Wconversion produces wrong warning

[Richard Guenther]

On Feb 16, 2008 6:31 PM, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
> On 16/02/2008, Richard Guenther <richard.guenther@gmail.com> wrote:
> >
> > The correct fix is to not fold expressions in the C frontend before doing
> > these kinds of warnings.
> >
>
> The fold is done by the last call to "convert" in build_binary_op in
> c-typeck.c And this is not just the C front end but also C++.
>
> So I don't see how folding can be prevented here.

It was mainly a general remark that the frontends should not fold
expressions but
defer that to the middle-end (we for example can (and do) fold during
gimplification).

Richard.

Re: [RFC] PR34389 -Wconversion produces wrong warning

[Manuel López-Ibáñez]

On 16/02/2008, Richard Guenther <richard.guenther@gmail.com> wrote:
>
> It was mainly a general remark that the frontends should not fold
> expressions but
> defer that to the middle-end (we for example can (and do) fold during
> gimplification).

Is that a general goal? a common opinion? Because I see the C front
end calling "fold*" functions everywhere, so it doesn't look like an
unintentional mistake...

Part of convert() in c-convert.c:

 if (code == VOID_TYPE)
    return fold_convert (type, e);
  if (code == INTEGER_TYPE || code == ENUMERAL_TYPE)
    return fold (convert_to_integer (type, e));
  if (code == BOOLEAN_TYPE)
    return fold_convert (type, c_objc_common_truthvalue_conversion (expr));
  if (code == POINTER_TYPE || code == REFERENCE_TYPE)
    return fold (convert_to_pointer (type, e));
  if (code == REAL_TYPE)
    return fold (convert_to_real (type, e));
  if (code == FIXED_POINT_TYPE)
    return fold (convert_to_fixed (type, e));
  if (code == COMPLEX_TYPE)
    return fold (convert_to_complex (type, e));
  if (code == VECTOR_TYPE)
    return fold (convert_to_vector (type, e));

Cheers,

Manuel.

Re: [RFC] PR34389 -Wconversion produces wrong warning

[Richard Guenther]

On Feb 16, 2008 7:06 PM, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
> On 16/02/2008, Richard Guenther <richard.guenther@gmail.com> wrote:
> >
> > It was mainly a general remark that the frontends should not fold
> > expressions but
> > defer that to the middle-end (we for example can (and do) fold during
> > gimplification).
>
> Is that a general goal? a common opinion? Because I see the C front
> end calling "fold*" functions everywhere, so it doesn't look like an
> unintentional mistake...
>
> Part of convert() in c-convert.c:
>
>  if (code == VOID_TYPE)
>     return fold_convert (type, e);
>   if (code == INTEGER_TYPE || code == ENUMERAL_TYPE)
>     return fold (convert_to_integer (type, e));
>   if (code == BOOLEAN_TYPE)
>     return fold_convert (type, c_objc_common_truthvalue_conversion (expr));
>   if (code == POINTER_TYPE || code == REFERENCE_TYPE)
>     return fold (convert_to_pointer (type, e));
>   if (code == REAL_TYPE)
>     return fold (convert_to_real (type, e));
>   if (code == FIXED_POINT_TYPE)
>     return fold (convert_to_fixed (type, e));
>   if (code == COMPLEX_TYPE)
>     return fold (convert_to_complex (type, e));
>   if (code == VECTOR_TYPE)
>     return fold (convert_to_vector (type, e));

It is a general goal that requires lots of work to make sure we do not regress
in optimization capabilities.  Basically frontends should only do simplification
of constants where it is required by the standard (such as to
determine if something
is a valid integral constant expression);  all other simplifications
should be defered
to the middle-end to make the IL emitted by the FE as close to the source input
as possible.

Now, this requires splitting the (shared between the FEs and middle-end) fold()
functions into a FE fold and a middle-end fold.  It also requires to
move existing
optimizations that only take place in FE specific code (like
convert.c) to places
in the middle-end.

It's a lot of work, but in the end it will be necessary - somewhen.

I considered cp fold-const.c gimple-fold.c a few times as a start (for example
look at the funny tests for the frontend name in fold-const.c - ugh).

Richard.

Re: [RFC] PR34389 -Wconversion produces wrong warning

[Manuel López-Ibáñez]

On 16/02/2008, Richard Guenther <richard.guenther@gmail.com> wrote:
> It's a lot of work, but in the end it will be necessary - somewhen.

So my patch is the only fix that could be available for GCC 4.3, and
probably for GCC 4.4 as well.

I can add some 'FIXME:' and '????' explaining all this but I don't see
any other workaround. So I still would like the patch to be applied to
fix this bug in a new feature.

Cheers,

Manuel.

Re: [RFC] PR34389 -Wconversion produces wrong warning

[Richard Guenther]

On Feb 17, 2008 5:41 PM, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
> On 16/02/2008, Richard Guenther <richard.guenther@gmail.com> wrote:
> > It's a lot of work, but in the end it will be necessary - somewhen.
>
> So my patch is the only fix that could be available for GCC 4.3, and
> probably for GCC 4.4 as well.
>
> I can add some 'FIXME:' and '????' explaining all this but I don't see
> any other workaround. So I still would like the patch to be applied to
> fix this bug in a new feature.

I don't think it is a great idea to stick such special-case
workarounds in.  After
all, the same situation (may) occurs with other operators than '&'.

But that's my 2cents only, a C frontend maintainer can override me
here of course.

Richard.

Re: [RFC] PR34389 -Wconversion produces wrong warning

[Joseph S. Myers]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1678 bytes --]

On Sat, 16 Feb 2008, Richard Guenther wrote:

> On Feb 16, 2008 5:52 PM, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
> > The following patch  fixes PR 34389 in a ugly way, in my own opinion,
> > but I am not able to come up with any other way to fix this.
> >
> > A more detailed analysis is in the PR. In summary the problem here is
> > that fold_unary converts (T)(x & c) into (T)x & (T)c which in this
> > case is (int) (x & 0x7fff) into ((int)x & 0x7fff). By using
> > get_unwidened, we could handle the first form. However, get_unwidened
> > won't handle the latter. Thus, when Wconversion sees that this
> > expression is assigned to a short, it concludes that there may be a
> > loss of precision.
> >
> > Bootstrapped and regression tested in x86_64-unknown-linux-gnu. This
> > patch only affects Wconversion, which is new in 4.3.
> 
> The correct fix is to not fold expressions in the C frontend before doing
> these kinds of warnings.

I agree with that in principle - but I also think it's useful to avoid 
warning for such cases as short = (int & 0x7fff) when written directly by 
the user (with any non-negative constant in the range of short there).  I 
suppose there are other cases where it may be less likely that the user 
will write the code created by the compiler, but anything affecting 
expressions directly written by the user would still be of some use after 
the front end no longer does the present folding.  (This means there 
should be testcases where the problem expressions are written directly by 
the user, not just the tests where they are compiler-generated.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [RFC] PR34389 -Wconversion produces wrong warning

[Manuel López-Ibáñez]

On 18/02/2008, Joseph S. Myers <joseph@codesourcery.com> wrote:
>
> I agree with that in principle - but I also think it's useful to avoid
> warning for such cases as short = (int & 0x7fff) when written directly by
> the user (with any non-negative constant in the range of short there).  I
> suppose there are other cases where it may be less likely that the user
> will write the code created by the compiler, but anything affecting
> expressions directly written by the user would still be of some use after
> the front end no longer does the present folding.  (This means there
> should be testcases where the problem expressions are written directly by
> the user, not just the tests where they are compiler-generated.)

My patch can handle

short x;
short y = ((int)x & 0x7fff)

because this case is covered in build_bin_op, where it is transformed
into (int)(x&0x7fff). So this case seems easy.

But it will still warn for

int x;
short y = x &  0x7fff;

Do we have any function or piece of code that tells when y = x & z
does not lose information or change a value? I really don't know what
the rules are for all combinations of precision/signedness of y, x and
z, and taking into account that each of them may contain an arbitrary
number of casts.

Cheers,

Manuel.

Re: [RFC] PR34389 -Wconversion produces wrong warning

[Manuel López-Ibáñez]

[-- Attachment #1: Type: text/plain, Size: 1760 bytes --]

On 18/02/2008, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
> On 18/02/2008, Joseph S. Myers <joseph@codesourcery.com> wrote:
> >
> > I agree with that in principle - but I also think it's useful to avoid
> > warning for such cases as short = (int & 0x7fff) when written directly by
> > the user (with any non-negative constant in the range of short there).  I
> > suppose there are other cases where it may be less likely that the user
> > will write the code created by the compiler, but anything affecting
> > expressions directly written by the user would still be of some use after
> > the front end no longer does the present folding.  (This means there
> > should be testcases where the problem expressions are written directly by
> > the user, not just the tests where they are compiler-generated.)

The following patch is a bit cleaner than the previous proposal.It
also handles a lot more of cases. In particular, it handles short =
(int & 0x7fff).

There is an XFAIL because common_types does completely different
things in C and C++. I think that is a latent bug exposed by my patch.

Let me know your opinions and suggestions.

Bootstrapped and regression tested in x86_64-unknown-linux-gnu.

Cheers,

Manuel.

2008-02-18  Manuel Lopez-Ibanez  <manu@gcc.gnu.org>

  PR 34389
  * c-typeck.c (build_binary_op): Encapsulate code into...
  * c-common.c (shorten_binary_op): ...this new function.
    (conversion_warning): Use the new function. Handle non-negative
constant in bitwise-and.
  * c-common.h (shorten_binary_op): Declare.
cp/
  * typeck.c (build_binary_op): Encapsulate code into the new function.
testsuite/
  * gcc.dg/Wconversion-pr34389.c: New.
  * g++.dg/warn/Wconversion-pr34389.C: New.

[-- Attachment #2: fix-pr34389.diff --]
[-- Type: text/plain, Size: 18709 bytes --]

Index: gcc/testsuite/gcc.dg/Wconversion-pr34389.c
===================================================================
--- gcc/testsuite/gcc.dg/Wconversion-pr34389.c	(revision 0)
+++ gcc/testsuite/gcc.dg/Wconversion-pr34389.c	(revision 0)
@@ -0,0 +1,53 @@
+/* PR 34389 */
+/* { dg-do compile } */
+/* { dg-options "-Wconversion -Wsign-conversion" } */
+
+short  mask1(short x)
+{
+  short y = 0x7fff;
+  return x & y;
+}
+
+short  mask2(short ssx)
+{
+  short ssy;
+  short ssz;
+
+  ssz = ((int)ssx) & 0x7fff;
+  ssy = ((int)ssx) | 0x7fff;
+  ssz = ((int)ssx) ^ 0x7fff;
+  return ssx & 0x7fff;
+}
+
+short  mask3(int si, unsigned int ui)
+{
+  short ss;
+  unsigned short us;
+
+  ss = si & 0x7fff;
+  ss = si & 0xAAAA; /* { dg-warning "conversion" } */
+  ss = ui & 0x7fff;
+  ss = ui & 0xAAAA; /* { dg-warning "conversion" } */
+
+  us = si & 0x7fff;
+  us = si & 0xAAAA; /* { dg-warning "conversion" } */
+  us = ui & 0x7fff;
+  us = ui & 0xAAAA; /* { dg-warning "conversion" } */
+
+  return ss;
+}
+
+short  mask4(int x, int y)
+{
+  return x & y; /* { dg-warning "conversion" } */
+}
+
+short  mask5(int x)
+{
+  return x & -1; /* { dg-warning "conversion" } */
+}
+
+short  mask6(short x)
+{
+  return x & -1;
+}
Index: gcc/testsuite/g++.dg/warn/Wconversion-pr34389.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Wconversion-pr34389.C	(revision 0)
+++ gcc/testsuite/g++.dg/warn/Wconversion-pr34389.C	(revision 0)
@@ -0,0 +1,53 @@
+/* PR 34389 */
+/* { dg-do compile } */
+/* { dg-options "-Wconversion -Wsign-conversion" } */
+
+short  mask1(short x)
+{
+  short y = 0x7fff;
+  return x & y; /* { dg-bogus "conversion" "conversion" { xfail *-*-* } 8 } */
+}
+
+short  mask2(short ssx)
+{
+  short ssy;
+  short ssz;
+
+  ssz = ((int)ssx) & 0x7fff;
+  ssy = ((int)ssx) | 0x7fff;
+  ssz = ((int)ssx) ^ 0x7fff;
+  return ssx & 0x7fff;
+}
+
+short  mask3(int si, unsigned int ui)
+{
+  short ss;
+  unsigned short us;
+
+  ss = si & 0x7fff;
+  ss = si & 0xAAAA; /* { dg-warning "conversion" } */
+  ss = ui & 0x7fff;
+  ss = ui & 0xAAAA; /* { dg-warning "conversion" } */
+
+  us = si & 0x7fff;
+  us = si & 0xAAAA; /* { dg-warning "conversion" } */
+  us = ui & 0x7fff;
+  us = ui & 0xAAAA; /* { dg-warning "conversion" } */
+
+  return ss;
+}
+
+short  mask4(int x, int y)
+{
+  return x & y; /* { dg-warning "conversion" } */
+}
+
+short  mask5(int x)
+{
+  return x & -1; /* { dg-warning "conversion" } */
+}
+
+short  mask6(short x)
+{
+  return x & -1;
+}
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 132310)
+++ gcc/cp/typeck.c	(working copy)
@@ -3656,65 +3656,13 @@ build_binary_op (enum tree_code code, tr
 	 Eg, (short)-1 | (unsigned short)-1 is (int)-1
 	 but calculated in (unsigned short) it would be (unsigned short)-1.  */
 
       if (shorten && none_complex)
 	{
-	  int unsigned0, unsigned1;
-	  tree arg0 = get_narrower (op0, &unsigned0);
-	  tree arg1 = get_narrower (op1, &unsigned1);
-	  /* UNS is 1 if the operation to be done is an unsigned one.  */
-	  int uns = TYPE_UNSIGNED (result_type);
-	  tree type;
-
 	  final_type = result_type;
-
-	  /* Handle the case that OP0 does not *contain* a conversion
-	     but it *requires* conversion to FINAL_TYPE.  */
-
-	  if (op0 == arg0 && TREE_TYPE (op0) != final_type)
-	    unsigned0 = TYPE_UNSIGNED (TREE_TYPE (op0));
-	  if (op1 == arg1 && TREE_TYPE (op1) != final_type)
-	    unsigned1 = TYPE_UNSIGNED (TREE_TYPE (op1));
-
-	  /* Now UNSIGNED0 is 1 if ARG0 zero-extends to FINAL_TYPE.  */
-
-	  /* For bitwise operations, signedness of nominal type
-	     does not matter.  Consider only how operands were extended.  */
-	  if (shorten == -1)
-	    uns = unsigned0;
-
-	  /* Note that in all three cases below we refrain from optimizing
-	     an unsigned operation on sign-extended args.
-	     That would not be valid.  */
-
-	  /* Both args variable: if both extended in same way
-	     from same width, do it in that width.
-	     Do it unsigned if args were zero-extended.  */
-	  if ((TYPE_PRECISION (TREE_TYPE (arg0))
-	       < TYPE_PRECISION (result_type))
-	      && (TYPE_PRECISION (TREE_TYPE (arg1))
-		  == TYPE_PRECISION (TREE_TYPE (arg0)))
-	      && unsigned0 == unsigned1
-	      && (unsigned0 || !uns))
-	    result_type = c_common_signed_or_unsigned_type
-	      (unsigned0, common_type (TREE_TYPE (arg0), TREE_TYPE (arg1)));
-	  else if (TREE_CODE (arg0) == INTEGER_CST
-		   && (unsigned1 || !uns)
-		   && (TYPE_PRECISION (TREE_TYPE (arg1))
-		       < TYPE_PRECISION (result_type))
-		   && (type = c_common_signed_or_unsigned_type
-		       (unsigned1, TREE_TYPE (arg1)),
-		       int_fits_type_p (arg0, type)))
-	    result_type = type;
-	  else if (TREE_CODE (arg1) == INTEGER_CST
-		   && (unsigned0 || !uns)
-		   && (TYPE_PRECISION (TREE_TYPE (arg0))
-		       < TYPE_PRECISION (result_type))
-		   && (type = c_common_signed_or_unsigned_type
-		       (unsigned0, TREE_TYPE (arg0)),
-		       int_fits_type_p (arg1, type)))
-	    result_type = type;
+	  result_type = shorten_binary_op (result_type, op0, op1, 
+					   shorten == -1);
 	}
 
       /* Comparison operations are shortened too but differently.
 	 They identify themselves by setting short_compare = 1.  */
 
Index: gcc/c-typeck.c
===================================================================
--- gcc/c-typeck.c	(revision 132310)
+++ gcc/c-typeck.c	(working copy)
@@ -8304,97 +8304,13 @@ build_binary_op (enum tree_code code, tr
 	 Eg, (short)-1 | (unsigned short)-1 is (int)-1
 	 but calculated in (unsigned short) it would be (unsigned short)-1.  */
 
       if (shorten && none_complex)
 	{
-	  int unsigned0, unsigned1;
-	  tree arg0, arg1;
-	  int uns;
-	  tree type;
-
-	  /* Cast OP0 and OP1 to RESULT_TYPE.  Doing so prevents
-	     excessive narrowing when we call get_narrower below.  For
-	     example, suppose that OP0 is of unsigned int extended
-	     from signed char and that RESULT_TYPE is long long int.
-	     If we explicitly cast OP0 to RESULT_TYPE, OP0 would look
-	     like
-
-	       (long long int) (unsigned int) signed_char
-
-	     which get_narrower would narrow down to
-
-	       (unsigned int) signed char
-
-	     If we do not cast OP0 first, get_narrower would return
-	     signed_char, which is inconsistent with the case of the
-	     explicit cast.  */
-	  op0 = convert (result_type, op0);
-	  op1 = convert (result_type, op1);
-
-	  arg0 = get_narrower (op0, &unsigned0);
-	  arg1 = get_narrower (op1, &unsigned1);
-
-	  /* UNS is 1 if the operation to be done is an unsigned one.  */
-	  uns = TYPE_UNSIGNED (result_type);
-
 	  final_type = result_type;
-
-	  /* Handle the case that OP0 (or OP1) does not *contain* a conversion
-	     but it *requires* conversion to FINAL_TYPE.  */
-
-	  if ((TYPE_PRECISION (TREE_TYPE (op0))
-	       == TYPE_PRECISION (TREE_TYPE (arg0)))
-	      && TREE_TYPE (op0) != final_type)
-	    unsigned0 = TYPE_UNSIGNED (TREE_TYPE (op0));
-	  if ((TYPE_PRECISION (TREE_TYPE (op1))
-	       == TYPE_PRECISION (TREE_TYPE (arg1)))
-	      && TREE_TYPE (op1) != final_type)
-	    unsigned1 = TYPE_UNSIGNED (TREE_TYPE (op1));
-
-	  /* Now UNSIGNED0 is 1 if ARG0 zero-extends to FINAL_TYPE.  */
-
-	  /* For bitwise operations, signedness of nominal type
-	     does not matter.  Consider only how operands were extended.  */
-	  if (shorten == -1)
-	    uns = unsigned0;
-
-	  /* Note that in all three cases below we refrain from optimizing
-	     an unsigned operation on sign-extended args.
-	     That would not be valid.  */
-
-	  /* Both args variable: if both extended in same way
-	     from same width, do it in that width.
-	     Do it unsigned if args were zero-extended.  */
-	  if ((TYPE_PRECISION (TREE_TYPE (arg0))
-	       < TYPE_PRECISION (result_type))
-	      && (TYPE_PRECISION (TREE_TYPE (arg1))
-		  == TYPE_PRECISION (TREE_TYPE (arg0)))
-	      && unsigned0 == unsigned1
-	      && (unsigned0 || !uns))
-	    result_type
-	      = c_common_signed_or_unsigned_type
-	      (unsigned0, common_type (TREE_TYPE (arg0), TREE_TYPE (arg1)));
-	  else if (TREE_CODE (arg0) == INTEGER_CST
-		   && (unsigned1 || !uns)
-		   && (TYPE_PRECISION (TREE_TYPE (arg1))
-		       < TYPE_PRECISION (result_type))
-		   && (type
-		       = c_common_signed_or_unsigned_type (unsigned1,
-							   TREE_TYPE (arg1)))
-		   && !POINTER_TYPE_P (type)
-		   && int_fits_type_p (arg0, type))
-	    result_type = type;
-	  else if (TREE_CODE (arg1) == INTEGER_CST
-		   && (unsigned0 || !uns)
-		   && (TYPE_PRECISION (TREE_TYPE (arg0))
-		       < TYPE_PRECISION (result_type))
-		   && (type
-		       = c_common_signed_or_unsigned_type (unsigned0,
-							   TREE_TYPE (arg0)))
-		   && !POINTER_TYPE_P (type)
-		   && int_fits_type_p (arg1, type))
-	    result_type = type;
+	  result_type = shorten_binary_op (result_type, op0, op1, 
+					   shorten == -1);
 	}
 
       /* Shifts can be shortened if shifting right.  */
 
       if (short_shift)
Index: gcc/c-common.c
===================================================================
--- gcc/c-common.c	(revision 132310)
+++ gcc/c-common.c	(working copy)
@@ -1206,10 +1206,114 @@ vector_types_convertible_p (const_tree t
     }
 
   return false;
 }
 
+/* This is a helper function of build_binary_op.
+
+   For certain operations if both args were extended from the same
+   smaller type, do the arithmetic in that type and then extend.
+
+   BITWISE indicates a bitwise operation.
+   For them, this optimization is safe only if
+   both args are zero-extended or both are sign-extended.
+   Otherwise, we might change the result.
+   Eg, (short)-1 | (unsigned short)-1 is (int)-1
+   but calculated in (unsigned short) it would be (unsigned short)-1.  
+*/
+tree shorten_binary_op (tree result_type, tree op0, tree op1, bool bitwise)
+{
+  int unsigned0, unsigned1;
+  tree arg0, arg1;
+  int uns;
+  tree type;
+
+  /* Cast OP0 and OP1 to RESULT_TYPE.  Doing so prevents
+     excessive narrowing when we call get_narrower below.  For
+     example, suppose that OP0 is of unsigned int extended
+     from signed char and that RESULT_TYPE is long long int.
+     If we explicitly cast OP0 to RESULT_TYPE, OP0 would look
+     like
+     
+     (long long int) (unsigned int) signed_char
+
+     which get_narrower would narrow down to
+     
+     (unsigned int) signed char
+     
+     If we do not cast OP0 first, get_narrower would return
+     signed_char, which is inconsistent with the case of the
+     explicit cast.  */
+  op0 = convert (result_type, op0);
+  op1 = convert (result_type, op1);
+
+  arg0 = get_narrower (op0, &unsigned0);
+  arg1 = get_narrower (op1, &unsigned1);
+
+  /* UNS is 1 if the operation to be done is an unsigned one.  */
+  uns = TYPE_UNSIGNED (result_type);
+
+  /* Handle the case that OP0 (or OP1) does not *contain* a conversion
+     but it *requires* conversion to FINAL_TYPE.  */
+  
+  if ((TYPE_PRECISION (TREE_TYPE (op0))
+       == TYPE_PRECISION (TREE_TYPE (arg0)))
+      && TREE_TYPE (op0) != result_type)
+    unsigned0 = TYPE_UNSIGNED (TREE_TYPE (op0));
+  if ((TYPE_PRECISION (TREE_TYPE (op1))
+       == TYPE_PRECISION (TREE_TYPE (arg1)))
+      && TREE_TYPE (op1) != result_type)
+    unsigned1 = TYPE_UNSIGNED (TREE_TYPE (op1));
+  
+  /* Now UNSIGNED0 is 1 if ARG0 zero-extends to FINAL_TYPE.  */
+  
+  /* For bitwise operations, signedness of nominal type
+     does not matter.  Consider only how operands were extended.  */
+  if (bitwise)
+    uns = unsigned0;
+  
+  /* Note that in all three cases below we refrain from optimizing
+     an unsigned operation on sign-extended args.
+     That would not be valid.  */
+  
+  /* Both args variable: if both extended in same way
+     from same width, do it in that width.
+     Do it unsigned if args were zero-extended.  */
+  if ((TYPE_PRECISION (TREE_TYPE (arg0))
+       < TYPE_PRECISION (result_type))
+      && (TYPE_PRECISION (TREE_TYPE (arg1))
+	  == TYPE_PRECISION (TREE_TYPE (arg0)))
+      && unsigned0 == unsigned1
+      && (unsigned0 || !uns))
+    return c_common_signed_or_unsigned_type
+      (unsigned0, common_type (TREE_TYPE (arg0), TREE_TYPE (arg1)));
+
+  else if (TREE_CODE (arg0) == INTEGER_CST
+	   && (unsigned1 || !uns)
+	   && (TYPE_PRECISION (TREE_TYPE (arg1))
+	       < TYPE_PRECISION (result_type))
+	   && (type
+	       = c_common_signed_or_unsigned_type (unsigned1,
+						   TREE_TYPE (arg1)))
+	   && !POINTER_TYPE_P (type)
+	   && int_fits_type_p (arg0, type))
+    return type;
+
+  else if (TREE_CODE (arg1) == INTEGER_CST
+	   && (unsigned0 || !uns)
+	   && (TYPE_PRECISION (TREE_TYPE (arg0))
+	       < TYPE_PRECISION (result_type))
+	   && (type
+	       = c_common_signed_or_unsigned_type (unsigned0,
+						   TREE_TYPE (arg0)))
+	   && !POINTER_TYPE_P (type)
+	   && int_fits_type_p (arg1, type))
+    return type;
+
+  return result_type;
+}
+
 /* Warns if the conversion of EXPR to TYPE may alter a value.
    This is a helper function for warnings_for_convert_and_check.  */
 
 static void
 conversion_warning (tree type, tree expr)
@@ -1270,65 +1374,96 @@ conversion_warning (tree type, tree expr
                  "conversion to %qT alters %qT constant value",
                  type, TREE_TYPE (expr));
     }
   else /* 'expr' is not a constant.  */
     {
+      tree expr_type = TREE_TYPE (expr);
+
       /* Warn for real types converted to integer types.  */
-      if (TREE_CODE (TREE_TYPE (expr)) == REAL_TYPE
+      if (TREE_CODE (expr_type) == REAL_TYPE
           && TREE_CODE (type) == INTEGER_TYPE)
         give_warning = true;
 
-      else if (TREE_CODE (TREE_TYPE (expr)) == INTEGER_TYPE
+      else if (TREE_CODE (expr_type) == INTEGER_TYPE
                && TREE_CODE (type) == INTEGER_TYPE)
         {
 	  /* Don't warn about unsigned char y = 0xff, x = (int) y;  */
 	  expr = get_unwidened (expr, 0);
+	  expr_type = TREE_TYPE (expr);
 
+	  /* Don't warn for short y; short x = ((int)y & 0xff);  */
+	  if (TREE_CODE (expr) == BIT_AND_EXPR 
+	      || TREE_CODE (expr) == BIT_IOR_EXPR 
+	      || TREE_CODE (expr) == BIT_XOR_EXPR)
+	    {
+	    /* It both args were extended from a shortest type, use
+	       that type if that is safe.  */
+	      expr_type = shorten_binary_op (expr_type, 
+					     TREE_OPERAND (expr, 0), 
+					     TREE_OPERAND (expr, 1), 
+					     /* bitwise */1);
+
+	      /* If one of the operands is a non-negative constant
+		 that fits in the target type, then the type of the
+		 other operand does not matter. */
+	      if (TREE_CODE (expr) == BIT_AND_EXPR)
+		{
+		  tree op0 = TREE_OPERAND (expr, 0);
+		  tree op1 = TREE_OPERAND (expr, 1);
+		  if ((TREE_CODE (op0) == INTEGER_CST
+		       && int_fits_type_p (op0, c_common_signed_type (type))
+		       && int_fits_type_p (op0, c_common_unsigned_type (type)))
+		      || (TREE_CODE (op1) == INTEGER_CST
+		       && int_fits_type_p (op1, c_common_signed_type (type))
+		       && int_fits_type_p (op1, c_common_unsigned_type (type))))
+		    return;
+		}
+	    }
           /* Warn for integer types converted to smaller integer types.  */
-          if (formal_prec < TYPE_PRECISION (TREE_TYPE (expr))) 
+          if (formal_prec < TYPE_PRECISION (expr_type)) 
 	    give_warning = true;
 
 	  /* When they are the same width but different signedness,
 	     then the value may change.  */
-	  else if ((formal_prec == TYPE_PRECISION (TREE_TYPE (expr))
-		    && TYPE_UNSIGNED (TREE_TYPE (expr)) != TYPE_UNSIGNED (type))
+	  else if ((formal_prec == TYPE_PRECISION (expr_type)
+		    && TYPE_UNSIGNED (expr_type) != TYPE_UNSIGNED (type))
 		   /* Even when converted to a bigger type, if the type is
 		      unsigned but expr is signed, then negative values
 		      will be changed.  */
-		   || (TYPE_UNSIGNED (type) && !TYPE_UNSIGNED (TREE_TYPE (expr))))
+		   || (TYPE_UNSIGNED (type) && !TYPE_UNSIGNED (expr_type)))
 	    warning (OPT_Wsign_conversion,
 		     "conversion to %qT from %qT may change the sign of the result",
-		     type, TREE_TYPE (expr));
+		     type, expr_type);
         }
 
       /* Warn for integer types converted to real types if and only if
          all the range of values of the integer type cannot be
          represented by the real type.  */
-      else if (TREE_CODE (TREE_TYPE (expr)) == INTEGER_TYPE
+      else if (TREE_CODE (expr_type) == INTEGER_TYPE
                && TREE_CODE (type) == REAL_TYPE)
         {
-          tree type_low_bound = TYPE_MIN_VALUE (TREE_TYPE (expr));
-          tree type_high_bound = TYPE_MAX_VALUE (TREE_TYPE (expr));
+          tree type_low_bound = TYPE_MIN_VALUE (expr_type);
+          tree type_high_bound = TYPE_MAX_VALUE (expr_type);
           REAL_VALUE_TYPE real_low_bound = real_value_from_int_cst (0, type_low_bound);
           REAL_VALUE_TYPE real_high_bound = real_value_from_int_cst (0, type_high_bound);
 
           if (!exact_real_truncate (TYPE_MODE (type), &real_low_bound)
               || !exact_real_truncate (TYPE_MODE (type), &real_high_bound))
             give_warning = true;
         }
 
       /* Warn for real types converted to smaller real types.  */
-      else if (TREE_CODE (TREE_TYPE (expr)) == REAL_TYPE
+      else if (TREE_CODE (expr_type) == REAL_TYPE
                && TREE_CODE (type) == REAL_TYPE
-               && formal_prec < TYPE_PRECISION (TREE_TYPE (expr)))
+               && formal_prec < TYPE_PRECISION (expr_type))
         give_warning = true;
 
 
       if (give_warning)
         warning (OPT_Wconversion,
                  "conversion to %qT from %qT may alter its value",
-                 type, TREE_TYPE (expr));
+                 type, expr_type);
     }
 }
 
 /* Produce warnings after a conversion. RESULT is the result of
    converting EXPR to TYPE.  This is a helper function for
Index: gcc/c-common.h
===================================================================
--- gcc/c-common.h	(revision 132310)
+++ gcc/c-common.h	(working copy)
@@ -699,10 +699,13 @@ extern bool c_determine_visibility (tree
 extern bool same_scalar_type_ignoring_signedness (tree, tree);
 
 #define c_sizeof(T)  c_sizeof_or_alignof_type (T, true, 1)
 #define c_alignof(T) c_sizeof_or_alignof_type (T, false, 1)
 
+/* Subroutine of build_binary_op, used for certain operations.  */
+extern tree shorten_binary_op (tree result_type, tree op0, tree op1, bool bitwise);
+
 /* Subroutine of build_binary_op, used for comparison operations.
    See if the operands have both been converted from subword integer types
    and, if so, perhaps change them both back to their original type.  */
 extern tree shorten_compare (tree *, tree *, tree *, enum tree_code *);
 

Re: [RFC] PR34389 -Wconversion produces wrong warning

[Richard Guenther]

On Feb 18, 2008 6:06 PM, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
> On 18/02/2008, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
> > On 18/02/2008, Joseph S. Myers <joseph@codesourcery.com> wrote:
> > >
> > > I agree with that in principle - but I also think it's useful to avoid
> > > warning for such cases as short = (int & 0x7fff) when written directly by
> > > the user (with any non-negative constant in the range of short there).  I
> > > suppose there are other cases where it may be less likely that the user
> > > will write the code created by the compiler, but anything affecting
> > > expressions directly written by the user would still be of some use after
> > > the front end no longer does the present folding.  (This means there
> > > should be testcases where the problem expressions are written directly by
> > > the user, not just the tests where they are compiler-generated.)
>
> The following patch is a bit cleaner than the previous proposal.It
> also handles a lot more of cases. In particular, it handles short =
> (int & 0x7fff).
>
> There is an XFAIL because common_types does completely different
> things in C and C++. I think that is a latent bug exposed by my patch.
>
> Let me know your opinions and suggestions.

Can you instead completely remove the shorten_binary_op code?  This
transformation is already done by fold and thus this code is redundant
(and optimization should not happen inside the frontend).

Thanks,
Richard.

Re: [RFC] PR34389 -Wconversion produces wrong warning

[Manuel López-Ibáñez]

[-- Attachment #1: Type: text/plain, Size: 1075 bytes --]

On 18/02/2008, Richard Guenther <richard.guenther@gmail.com> wrote:
>
> Can you instead completely remove the shorten_binary_op code?  This
> transformation is already done by fold and thus this code is redundant
> (and optimization should not happen inside the frontend).

Done!

Bootstrapped and regression tested in x86_64-unknown-linux-gnu.

I keep the function just for conversion_warning, since, as Joseph
explained, the extended operation may be user code and not the result
of folding, so we need to deal with it. I guess that for this purpose
the function can be greatly simplified, but I would like to know
whether this is the right approach.


2008-02-18  Manuel Lopez-Ibanez  <manu@gcc.gnu.org>

 PR 34389
 * c-typeck.c (build_binary_op): Delete optimization.
 * c-common.c (shorten_binary_op): New helper static function.
   (conversion_warning): Use the new function. Handle non-negative
constant in bitwise-and.
cp/
 * typeck.c (build_binary_op): Delete optimization.
testsuite/
 * gcc.dg/Wconversion-pr34389.c: New.
 * g++.dg/warn/Wconversion-pr34389.C: New.

[-- Attachment #2: remove-shorten-binary-op.diff --]
[-- Type: text/plain, Size: 17711 bytes --]

Index: gcc/testsuite/gcc.dg/Wconversion-pr34389.c
===================================================================
--- gcc/testsuite/gcc.dg/Wconversion-pr34389.c	(revision 0)
+++ gcc/testsuite/gcc.dg/Wconversion-pr34389.c	(revision 0)
@@ -0,0 +1,53 @@
+/* PR 34389 */
+/* { dg-do compile } */
+/* { dg-options "-Wconversion -Wsign-conversion" } */
+
+short  mask1(short x)
+{
+  short y = 0x7fff;
+  return x & y;
+}
+
+short  mask2(short ssx)
+{
+  short ssy;
+  short ssz;
+
+  ssz = ((int)ssx) & 0x7fff;
+  ssy = ((int)ssx) | 0x7fff;
+  ssz = ((int)ssx) ^ 0x7fff;
+  return ssx & 0x7fff;
+}
+
+short  mask3(int si, unsigned int ui)
+{
+  short ss;
+  unsigned short us;
+
+  ss = si & 0x7fff;
+  ss = si & 0xAAAA; /* { dg-warning "conversion" } */
+  ss = ui & 0x7fff;
+  ss = ui & 0xAAAA; /* { dg-warning "conversion" } */
+
+  us = si & 0x7fff;
+  us = si & 0xAAAA; /* { dg-warning "conversion" } */
+  us = ui & 0x7fff;
+  us = ui & 0xAAAA; /* { dg-warning "conversion" } */
+
+  return ss;
+}
+
+short  mask4(int x, int y)
+{
+  return x & y; /* { dg-warning "conversion" } */
+}
+
+short  mask5(int x)
+{
+  return x & -1; /* { dg-warning "conversion" } */
+}
+
+short  mask6(short x)
+{
+  return x & -1;
+}
Index: gcc/testsuite/g++.dg/warn/Wconversion-pr34389.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Wconversion-pr34389.C	(revision 0)
+++ gcc/testsuite/g++.dg/warn/Wconversion-pr34389.C	(revision 0)
@@ -0,0 +1,53 @@
+/* PR 34389 */
+/* { dg-do compile } */
+/* { dg-options "-Wconversion -Wsign-conversion" } */
+
+short  mask1(short x)
+{
+  short y = 0x7fff;
+  return x & y; /* { dg-bogus "conversion" "conversion" { xfail *-*-* } 8 } */
+}
+
+short  mask2(short ssx)
+{
+  short ssy;
+  short ssz;
+
+  ssz = ((int)ssx) & 0x7fff;
+  ssy = ((int)ssx) | 0x7fff;
+  ssz = ((int)ssx) ^ 0x7fff;
+  return ssx & 0x7fff;
+}
+
+short  mask3(int si, unsigned int ui)
+{
+  short ss;
+  unsigned short us;
+
+  ss = si & 0x7fff;
+  ss = si & 0xAAAA; /* { dg-warning "conversion" } */
+  ss = ui & 0x7fff;
+  ss = ui & 0xAAAA; /* { dg-warning "conversion" } */
+
+  us = si & 0x7fff;
+  us = si & 0xAAAA; /* { dg-warning "conversion" } */
+  us = ui & 0x7fff;
+  us = ui & 0xAAAA; /* { dg-warning "conversion" } */
+
+  return ss;
+}
+
+short  mask4(int x, int y)
+{
+  return x & y; /* { dg-warning "conversion" } */
+}
+
+short  mask5(int x)
+{
+  return x & -1; /* { dg-warning "conversion" } */
+}
+
+short  mask6(short x)
+{
+  return x & -1;
+}
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 132310)
+++ gcc/cp/typeck.c	(working copy)
@@ -3656,65 +3656,11 @@ build_binary_op (enum tree_code code, tr
 	 Eg, (short)-1 | (unsigned short)-1 is (int)-1
 	 but calculated in (unsigned short) it would be (unsigned short)-1.  */
 
       if (shorten && none_complex)
 	{
-	  int unsigned0, unsigned1;
-	  tree arg0 = get_narrower (op0, &unsigned0);
-	  tree arg1 = get_narrower (op1, &unsigned1);
-	  /* UNS is 1 if the operation to be done is an unsigned one.  */
-	  int uns = TYPE_UNSIGNED (result_type);
-	  tree type;
-
 	  final_type = result_type;
-
-	  /* Handle the case that OP0 does not *contain* a conversion
-	     but it *requires* conversion to FINAL_TYPE.  */
-
-	  if (op0 == arg0 && TREE_TYPE (op0) != final_type)
-	    unsigned0 = TYPE_UNSIGNED (TREE_TYPE (op0));
-	  if (op1 == arg1 && TREE_TYPE (op1) != final_type)
-	    unsigned1 = TYPE_UNSIGNED (TREE_TYPE (op1));
-
-	  /* Now UNSIGNED0 is 1 if ARG0 zero-extends to FINAL_TYPE.  */
-
-	  /* For bitwise operations, signedness of nominal type
-	     does not matter.  Consider only how operands were extended.  */
-	  if (shorten == -1)
-	    uns = unsigned0;
-
-	  /* Note that in all three cases below we refrain from optimizing
-	     an unsigned operation on sign-extended args.
-	     That would not be valid.  */
-
-	  /* Both args variable: if both extended in same way
-	     from same width, do it in that width.
-	     Do it unsigned if args were zero-extended.  */
-	  if ((TYPE_PRECISION (TREE_TYPE (arg0))
-	       < TYPE_PRECISION (result_type))
-	      && (TYPE_PRECISION (TREE_TYPE (arg1))
-		  == TYPE_PRECISION (TREE_TYPE (arg0)))
-	      && unsigned0 == unsigned1
-	      && (unsigned0 || !uns))
-	    result_type = c_common_signed_or_unsigned_type
-	      (unsigned0, common_type (TREE_TYPE (arg0), TREE_TYPE (arg1)));
-	  else if (TREE_CODE (arg0) == INTEGER_CST
-		   && (unsigned1 || !uns)
-		   && (TYPE_PRECISION (TREE_TYPE (arg1))
-		       < TYPE_PRECISION (result_type))
-		   && (type = c_common_signed_or_unsigned_type
-		       (unsigned1, TREE_TYPE (arg1)),
-		       int_fits_type_p (arg0, type)))
-	    result_type = type;
-	  else if (TREE_CODE (arg1) == INTEGER_CST
-		   && (unsigned0 || !uns)
-		   && (TYPE_PRECISION (TREE_TYPE (arg0))
-		       < TYPE_PRECISION (result_type))
-		   && (type = c_common_signed_or_unsigned_type
-		       (unsigned0, TREE_TYPE (arg0)),
-		       int_fits_type_p (arg1, type)))
-	    result_type = type;
 	}
 
       /* Comparison operations are shortened too but differently.
 	 They identify themselves by setting short_compare = 1.  */
 
Index: gcc/c-typeck.c
===================================================================
--- gcc/c-typeck.c	(revision 132310)
+++ gcc/c-typeck.c	(working copy)
@@ -8304,97 +8304,11 @@ build_binary_op (enum tree_code code, tr
 	 Eg, (short)-1 | (unsigned short)-1 is (int)-1
 	 but calculated in (unsigned short) it would be (unsigned short)-1.  */
 
       if (shorten && none_complex)
 	{
-	  int unsigned0, unsigned1;
-	  tree arg0, arg1;
-	  int uns;
-	  tree type;
-
-	  /* Cast OP0 and OP1 to RESULT_TYPE.  Doing so prevents
-	     excessive narrowing when we call get_narrower below.  For
-	     example, suppose that OP0 is of unsigned int extended
-	     from signed char and that RESULT_TYPE is long long int.
-	     If we explicitly cast OP0 to RESULT_TYPE, OP0 would look
-	     like
-
-	       (long long int) (unsigned int) signed_char
-
-	     which get_narrower would narrow down to
-
-	       (unsigned int) signed char
-
-	     If we do not cast OP0 first, get_narrower would return
-	     signed_char, which is inconsistent with the case of the
-	     explicit cast.  */
-	  op0 = convert (result_type, op0);
-	  op1 = convert (result_type, op1);
-
-	  arg0 = get_narrower (op0, &unsigned0);
-	  arg1 = get_narrower (op1, &unsigned1);
-
-	  /* UNS is 1 if the operation to be done is an unsigned one.  */
-	  uns = TYPE_UNSIGNED (result_type);
-
 	  final_type = result_type;
-
-	  /* Handle the case that OP0 (or OP1) does not *contain* a conversion
-	     but it *requires* conversion to FINAL_TYPE.  */
-
-	  if ((TYPE_PRECISION (TREE_TYPE (op0))
-	       == TYPE_PRECISION (TREE_TYPE (arg0)))
-	      && TREE_TYPE (op0) != final_type)
-	    unsigned0 = TYPE_UNSIGNED (TREE_TYPE (op0));
-	  if ((TYPE_PRECISION (TREE_TYPE (op1))
-	       == TYPE_PRECISION (TREE_TYPE (arg1)))
-	      && TREE_TYPE (op1) != final_type)
-	    unsigned1 = TYPE_UNSIGNED (TREE_TYPE (op1));
-
-	  /* Now UNSIGNED0 is 1 if ARG0 zero-extends to FINAL_TYPE.  */
-
-	  /* For bitwise operations, signedness of nominal type
-	     does not matter.  Consider only how operands were extended.  */
-	  if (shorten == -1)
-	    uns = unsigned0;
-
-	  /* Note that in all three cases below we refrain from optimizing
-	     an unsigned operation on sign-extended args.
-	     That would not be valid.  */
-
-	  /* Both args variable: if both extended in same way
-	     from same width, do it in that width.
-	     Do it unsigned if args were zero-extended.  */
-	  if ((TYPE_PRECISION (TREE_TYPE (arg0))
-	       < TYPE_PRECISION (result_type))
-	      && (TYPE_PRECISION (TREE_TYPE (arg1))
-		  == TYPE_PRECISION (TREE_TYPE (arg0)))
-	      && unsigned0 == unsigned1
-	      && (unsigned0 || !uns))
-	    result_type
-	      = c_common_signed_or_unsigned_type
-	      (unsigned0, common_type (TREE_TYPE (arg0), TREE_TYPE (arg1)));
-	  else if (TREE_CODE (arg0) == INTEGER_CST
-		   && (unsigned1 || !uns)
-		   && (TYPE_PRECISION (TREE_TYPE (arg1))
-		       < TYPE_PRECISION (result_type))
-		   && (type
-		       = c_common_signed_or_unsigned_type (unsigned1,
-							   TREE_TYPE (arg1)))
-		   && !POINTER_TYPE_P (type)
-		   && int_fits_type_p (arg0, type))
-	    result_type = type;
-	  else if (TREE_CODE (arg1) == INTEGER_CST
-		   && (unsigned0 || !uns)
-		   && (TYPE_PRECISION (TREE_TYPE (arg0))
-		       < TYPE_PRECISION (result_type))
-		   && (type
-		       = c_common_signed_or_unsigned_type (unsigned0,
-							   TREE_TYPE (arg0)))
-		   && !POINTER_TYPE_P (type)
-		   && int_fits_type_p (arg1, type))
-	    result_type = type;
 	}
 
       /* Shifts can be shortened if shifting right.  */
 
       if (short_shift)
Index: gcc/c-common.c
===================================================================
--- gcc/c-common.c	(revision 132310)
+++ gcc/c-common.c	(working copy)
@@ -1206,10 +1206,116 @@ vector_types_convertible_p (const_tree t
     }
 
   return false;
 }
 
+/* This is a helper function of conversion_warning.
+
+   For certain operations if both args were extended from the same
+   smaller type, the arithmetic can be safely done in that
+   type. Return that type.
+
+   BITWISE indicates a bitwise operation.
+   For them, this optimization is safe only if
+   both args are zero-extended or both are sign-extended.
+   Otherwise, we might change the result.
+   Eg, (short)-1 | (unsigned short)-1 is (int)-1
+   but calculated in (unsigned short) it would be (unsigned short)-1.  
+*/
+static tree 
+shorten_binary_op (tree result_type, tree op0, tree op1, bool bitwise)
+{
+  int unsigned0, unsigned1;
+  tree arg0, arg1;
+  int uns;
+  tree type;
+
+  /* Cast OP0 and OP1 to RESULT_TYPE.  Doing so prevents
+     excessive narrowing when we call get_narrower below.  For
+     example, suppose that OP0 is of unsigned int extended
+     from signed char and that RESULT_TYPE is long long int.
+     If we explicitly cast OP0 to RESULT_TYPE, OP0 would look
+     like
+     
+     (long long int) (unsigned int) signed_char
+
+     which get_narrower would narrow down to
+     
+     (unsigned int) signed char
+     
+     If we do not cast OP0 first, get_narrower would return
+     signed_char, which is inconsistent with the case of the
+     explicit cast.  */
+  op0 = convert (result_type, op0);
+  op1 = convert (result_type, op1);
+
+  arg0 = get_narrower (op0, &unsigned0);
+  arg1 = get_narrower (op1, &unsigned1);
+
+  /* UNS is 1 if the operation to be done is an unsigned one.  */
+  uns = TYPE_UNSIGNED (result_type);
+
+  /* Handle the case that OP0 (or OP1) does not *contain* a conversion
+     but it *requires* conversion to FINAL_TYPE.  */
+  
+  if ((TYPE_PRECISION (TREE_TYPE (op0))
+       == TYPE_PRECISION (TREE_TYPE (arg0)))
+      && TREE_TYPE (op0) != result_type)
+    unsigned0 = TYPE_UNSIGNED (TREE_TYPE (op0));
+  if ((TYPE_PRECISION (TREE_TYPE (op1))
+       == TYPE_PRECISION (TREE_TYPE (arg1)))
+      && TREE_TYPE (op1) != result_type)
+    unsigned1 = TYPE_UNSIGNED (TREE_TYPE (op1));
+  
+  /* Now UNSIGNED0 is 1 if ARG0 zero-extends to FINAL_TYPE.  */
+  
+  /* For bitwise operations, signedness of nominal type
+     does not matter.  Consider only how operands were extended.  */
+  if (bitwise)
+    uns = unsigned0;
+  
+  /* Note that in all three cases below we refrain from optimizing
+     an unsigned operation on sign-extended args.
+     That would not be valid.  */
+  
+  /* Both args variable: if both extended in same way
+     from same width, do it in that width.
+     Do it unsigned if args were zero-extended.  */
+  if ((TYPE_PRECISION (TREE_TYPE (arg0))
+       < TYPE_PRECISION (result_type))
+      && (TYPE_PRECISION (TREE_TYPE (arg1))
+	  == TYPE_PRECISION (TREE_TYPE (arg0)))
+      && unsigned0 == unsigned1
+      && (unsigned0 || !uns))
+    return c_common_signed_or_unsigned_type
+      (unsigned0, common_type (TREE_TYPE (arg0), TREE_TYPE (arg1)));
+
+  else if (TREE_CODE (arg0) == INTEGER_CST
+	   && (unsigned1 || !uns)
+	   && (TYPE_PRECISION (TREE_TYPE (arg1))
+	       < TYPE_PRECISION (result_type))
+	   && (type
+	       = c_common_signed_or_unsigned_type (unsigned1,
+						   TREE_TYPE (arg1)))
+	   && !POINTER_TYPE_P (type)
+	   && int_fits_type_p (arg0, type))
+    return type;
+
+  else if (TREE_CODE (arg1) == INTEGER_CST
+	   && (unsigned0 || !uns)
+	   && (TYPE_PRECISION (TREE_TYPE (arg0))
+	       < TYPE_PRECISION (result_type))
+	   && (type
+	       = c_common_signed_or_unsigned_type (unsigned0,
+						   TREE_TYPE (arg0)))
+	   && !POINTER_TYPE_P (type)
+	   && int_fits_type_p (arg1, type))
+    return type;
+
+  return result_type;
+}
+
 /* Warns if the conversion of EXPR to TYPE may alter a value.
    This is a helper function for warnings_for_convert_and_check.  */
 
 static void
 conversion_warning (tree type, tree expr)
@@ -1270,65 +1376,96 @@ conversion_warning (tree type, tree expr
                  "conversion to %qT alters %qT constant value",
                  type, TREE_TYPE (expr));
     }
   else /* 'expr' is not a constant.  */
     {
+      tree expr_type = TREE_TYPE (expr);
+
       /* Warn for real types converted to integer types.  */
-      if (TREE_CODE (TREE_TYPE (expr)) == REAL_TYPE
+      if (TREE_CODE (expr_type) == REAL_TYPE
           && TREE_CODE (type) == INTEGER_TYPE)
         give_warning = true;
 
-      else if (TREE_CODE (TREE_TYPE (expr)) == INTEGER_TYPE
+      else if (TREE_CODE (expr_type) == INTEGER_TYPE
                && TREE_CODE (type) == INTEGER_TYPE)
         {
 	  /* Don't warn about unsigned char y = 0xff, x = (int) y;  */
 	  expr = get_unwidened (expr, 0);
+	  expr_type = TREE_TYPE (expr);
 
+	  /* Don't warn for short y; short x = ((int)y & 0xff);  */
+	  if (TREE_CODE (expr) == BIT_AND_EXPR 
+	      || TREE_CODE (expr) == BIT_IOR_EXPR 
+	      || TREE_CODE (expr) == BIT_XOR_EXPR)
+	    {
+	    /* It both args were extended from a shortest type, use
+	       that type if that is safe.  */
+	      expr_type = shorten_binary_op (expr_type, 
+					     TREE_OPERAND (expr, 0), 
+					     TREE_OPERAND (expr, 1), 
+					     /* bitwise */1);
+
+	      /* If one of the operands is a non-negative constant
+		 that fits in the target type, then the type of the
+		 other operand does not matter. */
+	      if (TREE_CODE (expr) == BIT_AND_EXPR)
+		{
+		  tree op0 = TREE_OPERAND (expr, 0);
+		  tree op1 = TREE_OPERAND (expr, 1);
+		  if ((TREE_CODE (op0) == INTEGER_CST
+		       && int_fits_type_p (op0, c_common_signed_type (type))
+		       && int_fits_type_p (op0, c_common_unsigned_type (type)))
+		      || (TREE_CODE (op1) == INTEGER_CST
+		       && int_fits_type_p (op1, c_common_signed_type (type))
+		       && int_fits_type_p (op1, c_common_unsigned_type (type))))
+		    return;
+		}
+	    }
           /* Warn for integer types converted to smaller integer types.  */
-          if (formal_prec < TYPE_PRECISION (TREE_TYPE (expr))) 
+          if (formal_prec < TYPE_PRECISION (expr_type)) 
 	    give_warning = true;
 
 	  /* When they are the same width but different signedness,
 	     then the value may change.  */
-	  else if ((formal_prec == TYPE_PRECISION (TREE_TYPE (expr))
-		    && TYPE_UNSIGNED (TREE_TYPE (expr)) != TYPE_UNSIGNED (type))
+	  else if ((formal_prec == TYPE_PRECISION (expr_type)
+		    && TYPE_UNSIGNED (expr_type) != TYPE_UNSIGNED (type))
 		   /* Even when converted to a bigger type, if the type is
 		      unsigned but expr is signed, then negative values
 		      will be changed.  */
-		   || (TYPE_UNSIGNED (type) && !TYPE_UNSIGNED (TREE_TYPE (expr))))
+		   || (TYPE_UNSIGNED (type) && !TYPE_UNSIGNED (expr_type)))
 	    warning (OPT_Wsign_conversion,
 		     "conversion to %qT from %qT may change the sign of the result",
-		     type, TREE_TYPE (expr));
+		     type, expr_type);
         }
 
       /* Warn for integer types converted to real types if and only if
          all the range of values of the integer type cannot be
          represented by the real type.  */
-      else if (TREE_CODE (TREE_TYPE (expr)) == INTEGER_TYPE
+      else if (TREE_CODE (expr_type) == INTEGER_TYPE
                && TREE_CODE (type) == REAL_TYPE)
         {
-          tree type_low_bound = TYPE_MIN_VALUE (TREE_TYPE (expr));
-          tree type_high_bound = TYPE_MAX_VALUE (TREE_TYPE (expr));
+          tree type_low_bound = TYPE_MIN_VALUE (expr_type);
+          tree type_high_bound = TYPE_MAX_VALUE (expr_type);
           REAL_VALUE_TYPE real_low_bound = real_value_from_int_cst (0, type_low_bound);
           REAL_VALUE_TYPE real_high_bound = real_value_from_int_cst (0, type_high_bound);
 
           if (!exact_real_truncate (TYPE_MODE (type), &real_low_bound)
               || !exact_real_truncate (TYPE_MODE (type), &real_high_bound))
             give_warning = true;
         }
 
       /* Warn for real types converted to smaller real types.  */
-      else if (TREE_CODE (TREE_TYPE (expr)) == REAL_TYPE
+      else if (TREE_CODE (expr_type) == REAL_TYPE
                && TREE_CODE (type) == REAL_TYPE
-               && formal_prec < TYPE_PRECISION (TREE_TYPE (expr)))
+               && formal_prec < TYPE_PRECISION (expr_type))
         give_warning = true;
 
 
       if (give_warning)
         warning (OPT_Wconversion,
                  "conversion to %qT from %qT may alter its value",
-                 type, TREE_TYPE (expr));
+                 type, expr_type);
     }
 }
 
 /* Produce warnings after a conversion. RESULT is the result of
    converting EXPR to TYPE.  This is a helper function for

Re: [RFC] PR34389 -Wconversion produces wrong warning

[Jakub Jelinek]

On Mon, Feb 18, 2008 at 09:22:33PM +0100, Manuel López-Ibáñez wrote:
> On 18/02/2008, Richard Guenther <richard.guenther@gmail.com> wrote:
> >
> > Can you instead completely remove the shorten_binary_op code?  This
> > transformation is already done by fold and thus this code is redundant
> > (and optimization should not happen inside the frontend).
> 
> Done!
> 
> Bootstrapped and regression tested in x86_64-unknown-linux-gnu.
> 
> I keep the function just for conversion_warning, since, as Joseph
> explained, the extended operation may be user code and not the result
> of folding, so we need to deal with it. I guess that for this purpose
> the function can be greatly simplified, but I would like to know
> whether this is the right approach.

Can you add testcases to make sure the optimizations you are removing from
the FEs really happen at gimplification time?
I think testsuite/g**.dg/tree-ssa/* tests with -fdump-tree-gimple pattern
matching would be appropriate.

	Jakub

Re: [RFC] PR34389 -Wconversion produces wrong warning

[Manuel López-Ibáñez]

On 18/02/2008, Jakub Jelinek <jakub@redhat.com> wrote:

>
> Can you add testcases to make sure the optimizations you are removing from
> the FEs really happen at gimplification time?
> I think testsuite/g**.dg/tree-ssa/* tests with -fdump-tree-gimple pattern
> matching would be appropriate.

No, I can't because they don't happen. Fold actually performs the
opposite transformation, which is (T) (a & b) -> ((T)a & (T)b). The
code removed was pointless because fold was overriding it.

Cheers,

Manuel.