PATCH to shorten_compare -Wtype-limits handling
Manuel López-Ibáñez
lopezibanez@gmail.com
Wed Dec 9 18:45:00 GMT 2015
On 20 November 2015 at 22:28, Jeff Law <law@redhat.com> wrote:
>>> That was the overall plan and he posted a patch for that. But that patch
>>> didn't do the due diligence to verify that once the shortening code was
>>> made
>>> "pure" that we didn't regress on the quality of the code we generated.
>>
>>
>> I thought that the original plan was to make the warning code also use
>> match.pd. That is, that all folding, including FE folding, will be
>> match.pd-based. Has this changed?
>
> I don't think that's changed.
>
> Detangling the two is the first step. Once detangled we can then look to
> move the warning to a more suitable location -- right now it's in the C/C++
> front-ends, firing way too early. Instead it ought to be checked in gimple
> form, after match.pd canonicalization and simplifications.
Perhaps worth noting is that the warnings in shorten_compare are
basically trying to figure out if X CMP_OP Y can be folded to
true/false. They do not care about shortening/optimizing the types and
conversions. Perhaps an intermediate step would be to duplicate the
function into a warning version and an optimization version, making
the warning version pure as you say, call the warning version first,
then the optimizing one. Note that this is not exactly what you want
to have at the end and it adds a bit of duplication, but it gives a
clear separation between what is needed for warning and what is needed
for optimizing.This is step one.
Step two makes the warning version fold using match.pd, fix any
warning regressions. Note that the warning version does not really
need to be pure. If the comparison can be folded to true/false, there
is no much more optimization to be done. But I'm not sure how the FEs
are handling results from folding for warnings. Otherwise, one is
probably duplicating a bit of analysis between the warning and
optimizing functions. Not sure if the overhead will be measurable at
all once the warning version uses match.pd.
Step three moves the optimizing version to match.pd. Identifying
regressions here may be harder, but no harder than any other move of
optimizations to match.pd.
The attached patch implements step 1. Feel free to do as you please with it.
Cheers,
Manuel.
-------------- next part --------------
Index: c-common.c
===================================================================
--- c-common.c (revision 230753)
+++ c-common.c (working copy)
@@ -4419,10 +4419,251 @@ expr_original_type (tree expr)
{
STRIP_SIGN_NOPS (expr);
return TREE_TYPE (expr);
}
+static void
+warn_shorten_compare (location_t loc, tree op0, tree op1,
+ tree restype, enum tree_code rescode)
+{
+ if (!warn_type_limits)
+ return;
+
+ switch (rescode)
+ {
+ case NE_EXPR:
+ case EQ_EXPR:
+ case LT_EXPR:
+ case GT_EXPR:
+ case LE_EXPR:
+ case GE_EXPR:
+ break;
+
+ default:
+ return;
+ }
+
+ /* Throw away any conversions to wider types
+ already present in the operands. */
+ int unsignedp0, unsignedp1;
+ tree primop0 = c_common_get_narrower (op0, &unsignedp0);
+ tree primop1 = c_common_get_narrower (op1, &unsignedp1);
+
+ /* If primopN is first sign-extended from primopN's precision to opN's
+ precision, then zero-extended from opN's precision to
+ restype precision, shortenings might be invalid. */
+ if (TYPE_PRECISION (TREE_TYPE (primop0)) < TYPE_PRECISION (TREE_TYPE (op0))
+ && TYPE_PRECISION (TREE_TYPE (op0)) < TYPE_PRECISION (restype)
+ && !unsignedp0
+ && TYPE_UNSIGNED (TREE_TYPE (op0)))
+ primop0 = op0;
+ if (TYPE_PRECISION (TREE_TYPE (primop1)) < TYPE_PRECISION (TREE_TYPE (op1))
+ && TYPE_PRECISION (TREE_TYPE (op1)) < TYPE_PRECISION (restype)
+ && !unsignedp1
+ && TYPE_UNSIGNED (TREE_TYPE (op1)))
+ primop1 = op1;
+
+ /* Handle the case that OP0 does not *contain* a conversion
+ but it *requires* conversion to FINAL_TYPE. */
+ if (op0 == primop0 && TREE_TYPE (op0) != restype)
+ unsignedp0 = TYPE_UNSIGNED (TREE_TYPE (op0));
+ if (op1 == primop1 && TREE_TYPE (op1) != restype)
+ unsignedp1 = TYPE_UNSIGNED (TREE_TYPE (op1));
+
+ /* If one of the operands must be float, we do not warn. */
+ if (TREE_CODE (TREE_TYPE (primop0)) == REAL_TYPE
+ || TREE_CODE (TREE_TYPE (primop1)) == REAL_TYPE)
+ return;
+
+ /* If first arg is constant, swap the args (changing operation
+ so value is preserved), for canonicalization. Don't do this if
+ the second arg is 0. */
+
+ if (TREE_CONSTANT (primop0)
+ && !integer_zerop (primop1) && !real_zerop (primop1)
+ && !fixed_zerop (primop1))
+ {
+ std::swap (primop0, primop1);
+ std::swap (op0, op1);
+ std::swap (unsignedp0, unsignedp1);
+
+ switch (rescode)
+ {
+ case LT_EXPR:
+ rescode = GT_EXPR;
+ break;
+ case GT_EXPR:
+ rescode = LT_EXPR;
+ break;
+ case LE_EXPR:
+ rescode = GE_EXPR;
+ break;
+ case GE_EXPR:
+ rescode = LE_EXPR;
+ break;
+ default:
+ break;
+ }
+ }
+
+ /* If comparing an integer against a constant more bits wide, maybe we can
+ deduce a value of 1 or 0 independent of the data. Or else truncate the
+ constant now rather than extend the variable at run time.
+
+ This is only interesting if the constant is the wider arg.
+ Also, it is not safe if the constant is unsigned and the
+ variable arg is signed, since in this case the variable
+ would be sign-extended and then regarded as unsigned.
+ Our technique fails in this case because the lowest/highest
+ possible unsigned results don't follow naturally from the
+ lowest/highest possible values of the variable operand.
+ For just EQ_EXPR and NE_EXPR there is another technique that
+ could be used: see if the constant can be faithfully represented
+ in the other operand's type, by truncating it and reextending it
+ and see if that preserves the constant's value. */
+
+ if (TREE_CODE (primop0) != INTEGER_CST
+ /* Don't warn if it's from a (non-system) macro. */
+ && TREE_CODE (TREE_TYPE (primop0)) != FIXED_POINT_TYPE
+ && TREE_CODE (primop1) == INTEGER_CST
+ && TYPE_PRECISION (TREE_TYPE (primop0)) < TYPE_PRECISION (restype)
+ && !(from_macro_expansion_at (expansion_point_location_if_in_system_header
+ (EXPR_LOCATION (primop0)))))
+ {
+ /* true if comparison is nominally unsigned. */
+ bool unsignedp = TYPE_UNSIGNED (restype);
+
+ /* If primop0 was sign-extended and unsigned comparison specd,
+ we do a signed comparison using the signed type bounds.
+ But the comparison we output must be unsigned.
+
+ Also, for inequalities, VAL is no good; but if the signed
+ comparison had *any* fixed result, it follows that the
+ unsigned comparison just tests the sign in reverse
+ (positive values are LE, negative ones GE).
+ So we can generate an unsigned comparison
+ against an extreme value of the signed type. */
+
+ if (unsignedp && !unsignedp0)
+ switch (rescode)
+ {
+ case LT_EXPR:
+ case GE_EXPR:
+ case LE_EXPR:
+ case GT_EXPR:
+ return;
+
+ default:
+ restype = c_common_signed_type (restype);
+ break;
+ }
+
+ if (TREE_TYPE (primop1) != restype)
+ {
+ /* Convert primop1 to target type, but do not introduce
+ additional overflow. We know primop1 is an int_cst. */
+ primop1 = force_fit_type (restype,
+ wide_int::from
+ (primop1,
+ TYPE_PRECISION (restype),
+ TYPE_SIGN (TREE_TYPE (primop1))),
+ 0, TREE_OVERFLOW (primop1));
+ }
+
+ tree type = c_common_signed_or_unsigned_type (unsignedp0,
+ TREE_TYPE (primop0));
+ tree maxval = TYPE_MAX_VALUE (type);
+ tree minval = TYPE_MIN_VALUE (type);
+ if (type != restype)
+ {
+ minval = convert (restype, minval);
+ maxval = convert (restype, maxval);
+ }
+
+ bool min_gt = tree_int_cst_lt (primop1, minval);
+ bool max_gt = tree_int_cst_lt (primop1, maxval);
+ bool min_lt = tree_int_cst_lt (minval, primop1);
+ bool max_lt = tree_int_cst_lt (maxval, primop1);
+
+ tree val = 0;
+ switch (rescode)
+ {
+ case NE_EXPR:
+ if (max_lt || min_gt)
+ val = truthvalue_true_node;
+ break;
+ case EQ_EXPR:
+ if (max_lt || min_gt)
+ val = truthvalue_false_node;
+ break;
+ case LT_EXPR:
+ if (max_lt)
+ val = truthvalue_true_node;
+ if (!min_lt)
+ val = truthvalue_false_node;
+ break;
+ case GT_EXPR:
+ if (min_gt)
+ val = truthvalue_true_node;
+ if (!max_gt)
+ val = truthvalue_false_node;
+ break;
+ case LE_EXPR:
+ if (!max_gt)
+ val = truthvalue_true_node;
+ if (min_gt)
+ val = truthvalue_false_node;
+ break;
+ case GE_EXPR:
+ if (!min_lt)
+ val = truthvalue_true_node;
+ if (max_lt)
+ val = truthvalue_false_node;
+ break;
+
+ default:
+ gcc_unreachable ();
+ }
+
+ if (val == truthvalue_false_node)
+ warning_at (loc, OPT_Wtype_limits,
+ "comparison is always false due to limited range of data type");
+ else if (val == truthvalue_true_node)
+ warning_at (loc, OPT_Wtype_limits,
+ "comparison is always true due to limited range of data type");
+ }
+ /* Here we must do the comparison on the nominal type
+ using the args exactly as we received them. */
+ /* All unsigned values are >= 0, so we warn. However,
+ if OP0 is a constant that is >= 0, the signedness of
+ the comparison isn't an issue, so suppress the
+ warning. */
+ else if (integer_zerop (op1) && TYPE_UNSIGNED (restype)
+ && !(TREE_CODE (op0) == INTEGER_CST
+ && !TREE_OVERFLOW (convert (c_common_signed_type (restype),
+ op0)))
+ /* Do not warn for enumeration types. */
+ && (TREE_CODE (expr_original_type (op0)) != ENUMERAL_TYPE))
+ {
+ switch (rescode)
+ {
+ case GE_EXPR:
+ warning_at (loc, OPT_Wtype_limits,
+ "comparison of unsigned expression >= 0 is always true");
+ break;
+
+ case LT_EXPR:
+ warning_at (loc, OPT_Wtype_limits,
+ "comparison of unsigned expression < 0 is always false");
+ break;
+
+ default:
+ break;
+ }
+ }
+}
+
/* 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.
This function is also responsible for converting the two operands
to the proper common type for comparison.
@@ -4447,10 +4688,12 @@ shorten_compare (location_t loc, tree *o
int unsignedp0, unsignedp1;
int real1, real2;
tree primop0, primop1;
enum tree_code code = *rescode_ptr;
+ warn_shorten_compare (loc, op0, op1, *restype_ptr, code);
+
/* Throw away any conversions to wider types
already present in the operands. */
primop0 = c_common_get_narrower (op0, &unsignedp0);
primop1 = c_common_get_narrower (op1, &unsignedp1);
@@ -4648,24 +4891,10 @@ shorten_compare (location_t loc, tree *o
break;
}
type = c_common_unsigned_type (type);
}
- if (TREE_CODE (primop0) != INTEGER_CST
- /* Don't warn if it's from a (non-system) macro. */
- && !(from_macro_expansion_at
- (expansion_point_location_if_in_system_header
- (EXPR_LOCATION (primop0)))))
- {
- if (val == truthvalue_false_node)
- warning_at (loc, OPT_Wtype_limits,
- "comparison is always false due to limited range of data type");
- if (val == truthvalue_true_node)
- warning_at (loc, OPT_Wtype_limits,
- "comparison is always true due to limited range of data type");
- }
-
if (val != 0)
{
/* Don't forget to evaluate PRIMOP0 if it has side effects. */
if (TREE_SIDE_EFFECTS (primop0))
return build2 (COMPOUND_EXPR, TREE_TYPE (val), primop0, val);
@@ -4732,35 +4961,17 @@ shorten_compare (location_t loc, tree *o
if (!real1 && !real2 && integer_zerop (primop1)
&& TYPE_UNSIGNED (*restype_ptr))
{
tree value = 0;
- /* All unsigned values are >= 0, so we warn. However,
- if OP0 is a constant that is >= 0, the signedness of
- the comparison isn't an issue, so suppress the
- warning. */
- bool warn =
- warn_type_limits && !in_system_header_at (loc)
- && !(TREE_CODE (primop0) == INTEGER_CST
- && !TREE_OVERFLOW (convert (c_common_signed_type (type),
- primop0)))
- /* Do not warn for enumeration types. */
- && (TREE_CODE (expr_original_type (primop0)) != ENUMERAL_TYPE);
-
switch (code)
{
case GE_EXPR:
- if (warn)
- warning_at (loc, OPT_Wtype_limits,
- "comparison of unsigned expression >= 0 is always true");
value = truthvalue_true_node;
break;
case LT_EXPR:
- if (warn)
- warning_at (loc, OPT_Wtype_limits,
- "comparison of unsigned expression < 0 is always false");
value = truthvalue_false_node;
break;
default:
break;
More information about the Gcc-patches
mailing list