* [RFC] PR34389 -Wconversion produces wrong warning @ 2008-02-16 17:00 Manuel López-Ibáñez 2008-02-16 17:05 ` Richard Guenther 0 siblings, 1 reply; 15+ messages in thread From: Manuel López-Ibáñez @ 2008-02-16 17:00 UTC (permalink / raw) To: GCC Patches [-- 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] PR34389 -Wconversion produces wrong warning 2008-02-16 17:00 [RFC] PR34389 -Wconversion produces wrong warning Manuel López-Ibáñez @ 2008-02-16 17:05 ` Richard Guenther 2008-02-16 17:42 ` Manuel López-Ibáñez 2008-02-18 1:21 ` Joseph S. Myers 0 siblings, 2 replies; 15+ messages in thread From: Richard Guenther @ 2008-02-16 17:05 UTC (permalink / raw) To: Manuel López-Ibáñez; +Cc: GCC Patches 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] PR34389 -Wconversion produces wrong warning 2008-02-16 17:05 ` Richard Guenther @ 2008-02-16 17:42 ` Manuel López-Ibáñez 2008-02-16 17:50 ` Richard Guenther 2008-02-18 1:21 ` Joseph S. Myers 1 sibling, 1 reply; 15+ messages in thread From: Manuel López-Ibáñez @ 2008-02-16 17:42 UTC (permalink / raw) To: Richard Guenther; +Cc: GCC Patches 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] PR34389 -Wconversion produces wrong warning 2008-02-16 17:42 ` Manuel López-Ibáñez @ 2008-02-16 17:50 ` Richard Guenther 2008-02-16 18:31 ` Manuel López-Ibáñez 0 siblings, 1 reply; 15+ messages in thread From: Richard Guenther @ 2008-02-16 17:50 UTC (permalink / raw) To: Manuel López-Ibáñez; +Cc: GCC Patches 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] PR34389 -Wconversion produces wrong warning 2008-02-16 17:50 ` Richard Guenther @ 2008-02-16 18:31 ` Manuel López-Ibáñez 2008-02-16 18:53 ` Richard Guenther 0 siblings, 1 reply; 15+ messages in thread From: Manuel López-Ibáñez @ 2008-02-16 18:31 UTC (permalink / raw) To: Richard Guenther; +Cc: GCC Patches 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] PR34389 -Wconversion produces wrong warning 2008-02-16 18:31 ` Manuel López-Ibáñez @ 2008-02-16 18:53 ` Richard Guenther 2008-02-17 16:44 ` Manuel López-Ibáñez 0 siblings, 1 reply; 15+ messages in thread From: Richard Guenther @ 2008-02-16 18:53 UTC (permalink / raw) To: Manuel López-Ibáñez; +Cc: GCC Patches 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] PR34389 -Wconversion produces wrong warning 2008-02-16 18:53 ` Richard Guenther @ 2008-02-17 16:44 ` Manuel López-Ibáñez 2008-02-17 17:51 ` Richard Guenther 0 siblings, 1 reply; 15+ messages in thread From: Manuel López-Ibáñez @ 2008-02-17 16:44 UTC (permalink / raw) To: Richard Guenther; +Cc: GCC Patches 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] PR34389 -Wconversion produces wrong warning 2008-02-17 16:44 ` Manuel López-Ibáñez @ 2008-02-17 17:51 ` Richard Guenther 0 siblings, 0 replies; 15+ messages in thread From: Richard Guenther @ 2008-02-17 17:51 UTC (permalink / raw) To: Manuel López-Ibáñez; +Cc: GCC Patches 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] PR34389 -Wconversion produces wrong warning 2008-02-16 17:05 ` Richard Guenther 2008-02-16 17:42 ` Manuel López-Ibáñez @ 2008-02-18 1:21 ` Joseph S. Myers 2008-02-18 15:48 ` Manuel López-Ibáñez 1 sibling, 1 reply; 15+ messages in thread From: Joseph S. Myers @ 2008-02-18 1:21 UTC (permalink / raw) To: Richard Guenther; +Cc: Manuel López-Ibáñez, GCC Patches [-- 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] PR34389 -Wconversion produces wrong warning 2008-02-18 1:21 ` Joseph S. Myers @ 2008-02-18 15:48 ` Manuel López-Ibáñez 2008-02-18 17:10 ` Manuel López-Ibáñez 0 siblings, 1 reply; 15+ messages in thread From: Manuel López-Ibáñez @ 2008-02-18 15:48 UTC (permalink / raw) To: Joseph S. Myers; +Cc: GCC Patches 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] PR34389 -Wconversion produces wrong warning 2008-02-18 15:48 ` Manuel López-Ibáñez @ 2008-02-18 17:10 ` Manuel López-Ibáñez 2008-02-18 17:26 ` Richard Guenther 0 siblings, 1 reply; 15+ messages in thread From: Manuel López-Ibáñez @ 2008-02-18 17:10 UTC (permalink / raw) To: Joseph S. Myers; +Cc: GCC Patches [-- 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 *); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] PR34389 -Wconversion produces wrong warning 2008-02-18 17:10 ` Manuel López-Ibáñez @ 2008-02-18 17:26 ` Richard Guenther 2008-02-18 20:40 ` Manuel López-Ibáñez 0 siblings, 1 reply; 15+ messages in thread From: Richard Guenther @ 2008-02-18 17:26 UTC (permalink / raw) To: Manuel López-Ibáñez; +Cc: Joseph S. Myers, GCC Patches 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] PR34389 -Wconversion produces wrong warning 2008-02-18 17:26 ` Richard Guenther @ 2008-02-18 20:40 ` Manuel López-Ibáñez 2008-02-18 21:37 ` Jakub Jelinek 0 siblings, 1 reply; 15+ messages in thread From: Manuel López-Ibáñez @ 2008-02-18 20:40 UTC (permalink / raw) To: Richard Guenther; +Cc: Joseph S. Myers, GCC Patches [-- 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] PR34389 -Wconversion produces wrong warning 2008-02-18 20:40 ` Manuel López-Ibáñez @ 2008-02-18 21:37 ` Jakub Jelinek 2008-02-18 22:11 ` Manuel López-Ibáñez 0 siblings, 1 reply; 15+ messages in thread From: Jakub Jelinek @ 2008-02-18 21:37 UTC (permalink / raw) To: Manuel López-Ibáñez Cc: Richard Guenther, Joseph S. Myers, GCC Patches 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] PR34389 -Wconversion produces wrong warning 2008-02-18 21:37 ` Jakub Jelinek @ 2008-02-18 22:11 ` Manuel López-Ibáñez 0 siblings, 0 replies; 15+ messages in thread From: Manuel López-Ibáñez @ 2008-02-18 22:11 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Richard Guenther, Joseph S. Myers, GCC Patches 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2008-02-18 21:37 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2008-02-16 17:00 [RFC] PR34389 -Wconversion produces wrong warning Manuel López-Ibáñez 2008-02-16 17:05 ` Richard Guenther 2008-02-16 17:42 ` Manuel López-Ibáñez 2008-02-16 17:50 ` Richard Guenther 2008-02-16 18:31 ` Manuel López-Ibáñez 2008-02-16 18:53 ` Richard Guenther 2008-02-17 16:44 ` Manuel López-Ibáñez 2008-02-17 17:51 ` Richard Guenther 2008-02-18 1:21 ` Joseph S. Myers 2008-02-18 15:48 ` Manuel López-Ibáñez 2008-02-18 17:10 ` Manuel López-Ibáñez 2008-02-18 17:26 ` Richard Guenther 2008-02-18 20:40 ` Manuel López-Ibáñez 2008-02-18 21:37 ` Jakub Jelinek 2008-02-18 22:11 ` Manuel López-Ibáñez
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).