[TREE-SSA-CCP] Issue warning when folding condition

[TREE-SSA-CCP] Issue warning when folding condition

[Kugan Vivekanandarajah]

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

The testcase pr33738.C for warning fails with early-vrp patch. The
reason is, with early-vrp ccp2 is folding the comparison that used to
be folded in simplify_stmt_for_jump_threading. Since early-vrp does
not perform jump-threading is not optimized there.

Attached patch adds this warning to tree-ssa-ccp.c. We might also run
into some other similar issues in the future.

Bootstrapped and regression tested on x86_64-linux-gnu with no new regressions.

Is this OK for trunk?

Thanks,
Kugan

gcc/ChangeLog:

2016-08-18  Kugan Vivekanandarajah  <kuganv@linaro.org>

    * tree-ssa-ccp.c (ccp_fold_stmt): If the comparison is being folded
    and the operand on the LHS is being compared against a constant
    value that is outside of type limit, issue warning.

[-- Attachment #2: 0007-Add-warn_type_limits-to-ccp.patch --]
[-- Type: text/x-patch, Size: 2530 bytes --]

From 62152b5499233c2136dc388fd0322bab999f0cb4 Mon Sep 17 00:00:00 2001
From: Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org>
Date: Thu, 18 Aug 2016 20:29:38 +1000
Subject: [PATCH 7/7] Add warn_type_limits to ccp

---
 gcc/tree-ssa-ccp.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/gcc/tree-ssa-ccp.c b/gcc/tree-ssa-ccp.c
index 5d5386e..3c32045 100644
--- a/gcc/tree-ssa-ccp.c
+++ b/gcc/tree-ssa-ccp.c
@@ -142,6 +142,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "cfgloop.h"
 #include "stor-layout.h"
 #include "optabs-query.h"
+#include "diagnostic-core.h"
+#include "intl.h"
 
 
 /* Possible lattice values.  */
@@ -2147,7 +2149,11 @@ ccp_fold_stmt (gimple_stmt_iterator *gsi)
     case GIMPLE_COND:
       {
 	gcond *cond_stmt = as_a <gcond *> (stmt);
+	tree lhs = gimple_cond_lhs (stmt);
+	tree rhs = gimple_cond_rhs (stmt);
 	ccp_prop_value_t val;
+	wide_int min, max, rhs_val;
+	bool warn_limit = false;
 	/* Statement evaluation will handle type mismatches in constants
 	   more gracefully than the final propagation.  This allows us to
 	   fold more conditionals here.  */
@@ -2165,6 +2171,41 @@ ccp_fold_stmt (gimple_stmt_iterator *gsi)
 	    fprintf (dump_file, "\n");
 	  }
 
+	/* If the comparison is being folded and the operand on the LHS
+	   is being compared against a constant value that is outside of
+	   the natural range of LHSs type, then the predicate will
+	   always fold regardless of the value of LHS.  If -Wtype-limits
+	   was specified, emit a warning.  */
+	if (warn_type_limits
+	    && INTEGRAL_TYPE_P (TREE_TYPE (lhs))
+	    && (rhs = get_constant_value (rhs))
+	    && (TREE-code (rhs) == INTEGER_CST))
+	  {
+	    rhs_val = rhs;
+	    min = TYPE_MIN_VALUE (TREE_TYPE (lhs));
+	    max = TYPE_MAX_VALUE (TREE_TYPE (lhs));
+	    warn_limit = true;
+	  }
+
+	if (warn_limit
+	    && (wi::cmp (rhs_val, min, TYPE_SIGN (TREE_TYPE (lhs))) == -1
+		|| wi::cmp (max, rhs_val, TYPE_SIGN (TREE_TYPE (lhs))) == -1))
+	  {
+	    location_t location;
+
+	    if (!gimple_has_location (stmt))
+	      location = input_location;
+	    else
+	      location = gimple_location (stmt);
+
+	    warning_at (location, OPT_Wtype_limits,
+			integer_zerop (val.value)
+			? G_("comparison always false "
+			     "due to limited range of data type")
+			: G_("comparison always true "
+			     "due to limited range of data type"));
+	  }
+
 	if (integer_zerop (val.value))
 	  gimple_cond_make_false (cond_stmt);
 	else
-- 
2.7.4

Re: [TREE-SSA-CCP] Issue warning when folding condition

[Kugan Vivekanandarajah]

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

On 19 August 2016 at 12:09, Kugan Vivekanandarajah
<kugan.vivekanandarajah@linaro.org> wrote:
> The testcase pr33738.C for warning fails with early-vrp patch. The
> reason is, with early-vrp ccp2 is folding the comparison that used to
> be folded in simplify_stmt_for_jump_threading. Since early-vrp does
> not perform jump-threading is not optimized there.
>
> Attached patch adds this warning to tree-ssa-ccp.c. We might also run
> into some other similar issues in the future.

Sorry, I attached the wrong patch (with typo). Here is the correct one.

Thanks,
Kugan

>
> Bootstrapped and regression tested on x86_64-linux-gnu with no new regressions.
>
> Is this OK for trunk?
>
> Thanks,
> Kugan
>
> gcc/ChangeLog:
>
> 2016-08-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>     * tree-ssa-ccp.c (ccp_fold_stmt): If the comparison is being folded
>     and the operand on the LHS is being compared against a constant
>     value that is outside of type limit, issue warning.

[-- Attachment #2: 0007-Add-warn_type_limits-to-ccp.patch --]
[-- Type: text/x-patch, Size: 2530 bytes --]

From 942f0e331f7a92ac46cee8793aac07af74e541e6 Mon Sep 17 00:00:00 2001
From: Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org>
Date: Thu, 18 Aug 2016 20:29:38 +1000
Subject: [PATCH 7/7] Add warn_type_limits to ccp

---
 gcc/tree-ssa-ccp.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/gcc/tree-ssa-ccp.c b/gcc/tree-ssa-ccp.c
index 5d5386e..fce76ce 100644
--- a/gcc/tree-ssa-ccp.c
+++ b/gcc/tree-ssa-ccp.c
@@ -142,6 +142,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "cfgloop.h"
 #include "stor-layout.h"
 #include "optabs-query.h"
+#include "diagnostic-core.h"
+#include "intl.h"
 
 
 /* Possible lattice values.  */
@@ -2147,7 +2149,11 @@ ccp_fold_stmt (gimple_stmt_iterator *gsi)
     case GIMPLE_COND:
       {
 	gcond *cond_stmt = as_a <gcond *> (stmt);
+	tree lhs = gimple_cond_lhs (stmt);
+	tree rhs = gimple_cond_rhs (stmt);
 	ccp_prop_value_t val;
+	wide_int min, max, rhs_val;
+	bool warn_limit = false;
 	/* Statement evaluation will handle type mismatches in constants
 	   more gracefully than the final propagation.  This allows us to
 	   fold more conditionals here.  */
@@ -2165,6 +2171,41 @@ ccp_fold_stmt (gimple_stmt_iterator *gsi)
 	    fprintf (dump_file, "\n");
 	  }
 
+	/* If the comparison is being folded and the operand on the LHS
+	   is being compared against a constant value that is outside of
+	   the natural range of LHSs type, then the predicate will
+	   always fold regardless of the value of LHS.  If -Wtype-limits
+	   was specified, emit a warning.  */
+	if (warn_type_limits
+	    && INTEGRAL_TYPE_P (TREE_TYPE (lhs))
+	    && (rhs = get_constant_value (rhs))
+	    && (TREE_CODE (rhs) == INTEGER_CST))
+	  {
+	    rhs_val = rhs;
+	    min = TYPE_MIN_VALUE (TREE_TYPE (lhs));
+	    max = TYPE_MAX_VALUE (TREE_TYPE (lhs));
+	    warn_limit = true;
+	  }
+
+	if (warn_limit
+	    && (wi::cmp (rhs_val, min, TYPE_SIGN (TREE_TYPE (lhs))) == -1
+		|| wi::cmp (max, rhs_val, TYPE_SIGN (TREE_TYPE (lhs))) == -1))
+	  {
+	    location_t location;
+
+	    if (!gimple_has_location (stmt))
+	      location = input_location;
+	    else
+	      location = gimple_location (stmt);
+
+	    warning_at (location, OPT_Wtype_limits,
+			integer_zerop (val.value)
+			? G_("comparison always false "
+			     "due to limited range of data type")
+			: G_("comparison always true "
+			     "due to limited range of data type"));
+	  }
+
 	if (integer_zerop (val.value))
 	  gimple_cond_make_false (cond_stmt);
 	else
-- 
2.7.4

Re: [TREE-SSA-CCP] Issue warning when folding condition

[Richard Biener]

On Fri, 19 Aug 2016, Kugan Vivekanandarajah wrote:

> On 19 August 2016 at 12:09, Kugan Vivekanandarajah
> <kugan.vivekanandarajah@linaro.org> wrote:
> > The testcase pr33738.C for warning fails with early-vrp patch. The
> > reason is, with early-vrp ccp2 is folding the comparison that used to
> > be folded in simplify_stmt_for_jump_threading. Since early-vrp does
> > not perform jump-threading is not optimized there.
> >
> > Attached patch adds this warning to tree-ssa-ccp.c. We might also run
> > into some other similar issues in the future.
> 
> Sorry, I attached the wrong patch (with typo). Here is the correct one.

I think emitting this warning from GIMPLE optimizations is fundamentally
flawed and the warning should be removed there and put next to
the cases we alrady handle in c/c-common.c:shorten_compare (or in
FE specific code).  I see no reason why only VRP or CCP would
do the simplification for -fstrict-enums enums (thus it seems to be
missing from the generic comparison folders).

Richard.

> Thanks,
> Kugan
> 
> >
> > Bootstrapped and regression tested on x86_64-linux-gnu with no new regressions.
> >
> > Is this OK for trunk?
> >
> > Thanks,
> > Kugan
> >
> > gcc/ChangeLog:
> >
> > 2016-08-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
> >
> >     * tree-ssa-ccp.c (ccp_fold_stmt): If the comparison is being folded
> >     and the operand on the LHS is being compared against a constant
> >     value that is outside of type limit, issue warning.
> 
k

Re: [TREE-SSA-CCP] Issue warning when folding condition

[Jeff Law]

On 08/18/2016 08:09 PM, Kugan Vivekanandarajah wrote:
> The testcase pr33738.C for warning fails with early-vrp patch. The
> reason is, with early-vrp ccp2 is folding the comparison that used to
> be folded in simplify_stmt_for_jump_threading. Since early-vrp does
> not perform jump-threading is not optimized there.
>
> Attached patch adds this warning to tree-ssa-ccp.c. We might also run
> into some other similar issues in the future.
>
> Bootstrapped and regression tested on x86_64-linux-gnu with no new regressions.
>
> Is this OK for trunk?
>
> Thanks,
> Kugan
>
> gcc/ChangeLog:
>
> 2016-08-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>     * tree-ssa-ccp.c (ccp_fold_stmt): If the comparison is being folded
>     and the operand on the LHS is being compared against a constant
>     value that is outside of type limit, issue warning.
So just to be clear, early VRP simplifies thing in a way that allows CCP 
(rather than a later VRP) to do the propagation exposing the comparison 
against an out-of-range constant.  Right?

> @@ -2147,7 +2149,11 @@ ccp_fold_stmt (gimple_stmt_iterator *gsi)
>      case GIMPLE_COND:
>        {
>  	gcond *cond_stmt = as_a <gcond *> (stmt);
> +	tree lhs = gimple_cond_lhs (stmt);
> +	tree rhs = gimple_cond_rhs (stmt);
>  	ccp_prop_value_t val;
> +	wide_int min, max, rhs_val;
> +	bool warn_limit = false;
>  	/* Statement evaluation will handle type mismatches in constants
>  	   more gracefully than the final propagation.  This allows us to
>  	   fold more conditionals here.  */
> @@ -2165,6 +2171,41 @@ ccp_fold_stmt (gimple_stmt_iterator *gsi)
>  	    fprintf (dump_file, "\n");
>  	  }
>
> +	/* If the comparison is being folded and the operand on the LHS
> +	   is being compared against a constant value that is outside of
> +	   the natural range of LHSs type, then the predicate will
> +	   always fold regardless of the value of LHS.  If -Wtype-limits
> +	   was specified, emit a warning.  */
> +	if (warn_type_limits
> +	    && INTEGRAL_TYPE_P (TREE_TYPE (lhs))
> +	    && (rhs = get_constant_value (rhs))
> +	    && (TREE-code (rhs) == INTEGER_CST))
                 ^^^^^^^^^
I'm not sure how this could have bootstrapped & regression tested?!?

I'm OK with the general direction here, but I think you need to retest ;-)

Presumably by the type we get to this test the expression is in canonial 
form?  I'm thinking specifically about a case that may have started off 
looking like

if (x >= y)

Where we're able to find a constant for x and that constant is out of 
the range of what y can hold.


jeff

Re: [TREE-SSA-CCP] Issue warning when folding condition

[kugan]

Hi Jeff,

On 13/09/16 08:11, Jeff Law wrote:
> On 08/18/2016 08:09 PM, Kugan Vivekanandarajah wrote:
>> The testcase pr33738.C for warning fails with early-vrp patch. The
>> reason is, with early-vrp ccp2 is folding the comparison that used to
>> be folded in simplify_stmt_for_jump_threading. Since early-vrp does
>> not perform jump-threading is not optimized there.
>>
>> Attached patch adds this warning to tree-ssa-ccp.c. We might also run
>> into some other similar issues in the future.
>>
>> Bootstrapped and regression tested on x86_64-linux-gnu with no new regressions.
>>
>> Is this OK for trunk?
>>
>> Thanks,
>> Kugan
>>
>> gcc/ChangeLog:
>>
>> 2016-08-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>
>>     * tree-ssa-ccp.c (ccp_fold_stmt): If the comparison is being folded
>>     and the operand on the LHS is being compared against a constant
>>     value that is outside of type limit, issue warning.
> So just to be clear, early VRP simplifies thing in a way that allows CCP
> (rather than a later VRP) to do the propagation exposing the comparison
> against an out-of-range constant.  Right?
>

Yes, thats right.

>> @@ -2147,7 +2149,11 @@ ccp_fold_stmt (gimple_stmt_iterator *gsi)
>>      case GIMPLE_COND:
>>        {
>>  	gcond *cond_stmt = as_a <gcond *> (stmt);
>> +	tree lhs = gimple_cond_lhs (stmt);
>> +	tree rhs = gimple_cond_rhs (stmt);
>>  	ccp_prop_value_t val;
>> +	wide_int min, max, rhs_val;
>> +	bool warn_limit = false;
>>  	/* Statement evaluation will handle type mismatches in constants
>>  	   more gracefully than the final propagation.  This allows us to
>>  	   fold more conditionals here.  */
>> @@ -2165,6 +2171,41 @@ ccp_fold_stmt (gimple_stmt_iterator *gsi)
>>  	    fprintf (dump_file, "\n");
>>  	  }
>>
>> +	/* If the comparison is being folded and the operand on the LHS
>> +	   is being compared against a constant value that is outside of
>> +	   the natural range of LHSs type, then the predicate will
>> +	   always fold regardless of the value of LHS.  If -Wtype-limits
>> +	   was specified, emit a warning.  */
>> +	if (warn_type_limits
>> +	    && INTEGRAL_TYPE_P (TREE_TYPE (lhs))
>> +	    && (rhs = get_constant_value (rhs))
>> +	    && (TREE-code (rhs) == INTEGER_CST))
>                  ^^^^^^^^^
> I'm not sure how this could have bootstrapped & regression tested?!?
Sorry, I attached the wrong patch. I resent the correct patch just with 
this change at :
https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01382.html

>
> I'm OK with the general direction here, but I think you need to retest ;-)
>
> Presumably by the type we get to this test the expression is in canonial
> form?  I'm thinking specifically about a case that may have started off
> looking like
>
> if (x >= y)
>
> Where we're able to find a constant for x and that constant is out of
> the range of what y can hold.

I agree that it may not be in canonical form all the time, especially 
when variables becomes constant as in your example.

I will resend with this change.

Thanks,
Kugan
>
>
> jeff
>
>

Re: [TREE-SSA-CCP] Issue warning when folding condition

[kugan]

Hi Richard,


On 19/08/16 18:00, Richard Biener wrote:
> On Fri, 19 Aug 2016, Kugan Vivekanandarajah wrote:
>
>> On 19 August 2016 at 12:09, Kugan Vivekanandarajah
>> <kugan.vivekanandarajah@linaro.org> wrote:
>>> The testcase pr33738.C for warning fails with early-vrp patch. The
>>> reason is, with early-vrp ccp2 is folding the comparison that used to
>>> be folded in simplify_stmt_for_jump_threading. Since early-vrp does
>>> not perform jump-threading is not optimized there.
>>>
>>> Attached patch adds this warning to tree-ssa-ccp.c. We might also run
>>> into some other similar issues in the future.
>>
>> Sorry, I attached the wrong patch (with typo). Here is the correct one.
>
> I think emitting this warning from GIMPLE optimizations is fundamentally
> flawed and the warning should be removed there and put next to
> the cases we alrady handle in c/c-common.c:shorten_compare (or in
> FE specific code).  I see no reason why only VRP or CCP would
> do the simplification for -fstrict-enums enums (thus it seems to be
> missing from the generic comparison folders).
>
But, If I understand this correctly, I think we will not be able to fold 
all the cases we handle in FE. Therefore we will not be able to warn 
there. For very simple cases yes, but not for others.


Thanks,
Kugan

> Richard.
>
>> Thanks,
>> Kugan
>>
>>>
>>> Bootstrapped and regression tested on x86_64-linux-gnu with no new regressions.
>>>
>>> Is this OK for trunk?
>>>
>>> Thanks,
>>> Kugan
>>>
>>> gcc/ChangeLog:
>>>
>>> 2016-08-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>>
>>>     * tree-ssa-ccp.c (ccp_fold_stmt): If the comparison is being folded
>>>     and the operand on the LHS is being compared against a constant
>>>     value that is outside of type limit, issue warning.
>>
> k
>

Re: [TREE-SSA-CCP] Issue warning when folding condition

[Richard Biener]

On Tue, 13 Sep 2016, kugan wrote:

> Hi Richard,
> 
> 
> On 19/08/16 18:00, Richard Biener wrote:
> > On Fri, 19 Aug 2016, Kugan Vivekanandarajah wrote:
> > 
> > > On 19 August 2016 at 12:09, Kugan Vivekanandarajah
> > > <kugan.vivekanandarajah@linaro.org> wrote:
> > > > The testcase pr33738.C for warning fails with early-vrp patch. The
> > > > reason is, with early-vrp ccp2 is folding the comparison that used to
> > > > be folded in simplify_stmt_for_jump_threading. Since early-vrp does
> > > > not perform jump-threading is not optimized there.
> > > > 
> > > > Attached patch adds this warning to tree-ssa-ccp.c. We might also run
> > > > into some other similar issues in the future.
> > > 
> > > Sorry, I attached the wrong patch (with typo). Here is the correct one.
> > 
> > I think emitting this warning from GIMPLE optimizations is fundamentally
> > flawed and the warning should be removed there and put next to
> > the cases we alrady handle in c/c-common.c:shorten_compare (or in
> > FE specific code).  I see no reason why only VRP or CCP would
> > do the simplification for -fstrict-enums enums (thus it seems to be
> > missing from the generic comparison folders).
> > 
> But, If I understand this correctly, I think we will not be able to fold all
> the cases we handle in FE. Therefore we will not be able to warn there. For
> very simple cases yes, but not for others.

Sure.  But I do not see an issue with that.  With GIMPLE level warnings
you have the general issue that the warning may be only exposed by
optimization (like cross-unit inlining with LTO) and thus would be
considered a false positive.

Richard.

> 
> Thanks,
> Kugan
> 
> > Richard.
> > 
> > > Thanks,
> > > Kugan
> > > 
> > > > 
> > > > Bootstrapped and regression tested on x86_64-linux-gnu with no new
> > > > regressions.
> > > > 
> > > > Is this OK for trunk?
> > > > 
> > > > Thanks,
> > > > Kugan
> > > > 
> > > > gcc/ChangeLog:
> > > > 
> > > > 2016-08-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
> > > > 
> > > >     * tree-ssa-ccp.c (ccp_fold_stmt): If the comparison is being folded
> > > >     and the operand on the LHS is being compared against a constant
> > > >     value that is outside of type limit, issue warning.
> > > 
> > k
> > 
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [TREE-SSA-CCP] Issue warning when folding condition

[Jeff Law]

On 09/14/2016 03:00 AM, Richard Biener wrote:
> On Tue, 13 Sep 2016, kugan wrote:
>
>> Hi Richard,
>>
>>
>> On 19/08/16 18:00, Richard Biener wrote:
>>> On Fri, 19 Aug 2016, Kugan Vivekanandarajah wrote:
>>>
>>>> On 19 August 2016 at 12:09, Kugan Vivekanandarajah
>>>> <kugan.vivekanandarajah@linaro.org> wrote:
>>>>> The testcase pr33738.C for warning fails with early-vrp patch. The
>>>>> reason is, with early-vrp ccp2 is folding the comparison that used to
>>>>> be folded in simplify_stmt_for_jump_threading. Since early-vrp does
>>>>> not perform jump-threading is not optimized there.
>>>>>
>>>>> Attached patch adds this warning to tree-ssa-ccp.c. We might also run
>>>>> into some other similar issues in the future.
>>>>
>>>> Sorry, I attached the wrong patch (with typo). Here is the correct one.
>>>
>>> I think emitting this warning from GIMPLE optimizations is fundamentally
>>> flawed and the warning should be removed there and put next to
>>> the cases we alrady handle in c/c-common.c:shorten_compare (or in
>>> FE specific code).  I see no reason why only VRP or CCP would
>>> do the simplification for -fstrict-enums enums (thus it seems to be
>>> missing from the generic comparison folders).
>>>
>> But, If I understand this correctly, I think we will not be able to fold all
>> the cases we handle in FE. Therefore we will not be able to warn there. For
>> very simple cases yes, but not for others.
>
> Sure.  But I do not see an issue with that.  With GIMPLE level warnings
> you have the general issue that the warning may be only exposed by
> optimization (like cross-unit inlining with LTO) and thus would be
> considered a false positive.
I must have missed a piece of this conversation.

If someone is going to add more stuff to shorten_compare, please break 
that function into two pieces.  One which is non-destructive to the 
original trees and just emits warnings, the other which has the 
optimization.

There may well be code & work duplication, but that separation is one of 
the steps necessary to move the shortening bits out of the front-end and 
into match.pd.

jeff