Re: [PATCH] Verify that last argument of __builtin_expect_with_probability is a real cst (PR c/87811).

2018-11-08 Thread Martin Sebor 

On 11/07/2018 02:36 AM, Martin Liška wrote:

On 11/5/18 7:00 PM, Martin Sebor wrote:

On 11/01/2018 07:45 AM, Martin Liška wrote:

On 11/1/18 1:15 PM, Jakub Jelinek wrote:

On Thu, Nov 01, 2018 at 01:09:16PM +0100, Martin Liška wrote:

-range 0.0 to 1.0, inclusive.
+range 0.0 to 1.0, inclusive.  The @var{probability} argument must be
+a compiler time constant.


When you say must, I think error_at should be used rather than warning_at.
If others disagree I'm open for leaving it as is.


Error is fine for me as well.




@@ -2474,6 +2481,11 @@ expr_expected_value_1 (tree type, tree op0, enum 
tree_code code,
   *predictor = PRED_BUILTIN_EXPECT_WITH_PROBABILITY;
   *probability = probi;
 }
+  else
+  warning_at (gimple_location (def), 0,
+  "probability argument %qE must be a in the "
+  "range 0.0 to 1.0", prob);


Wrong indentation.

And, no diagnostics for -O0 (which should also be covered by a testcase).


Test for that added.




+/* { dg-options "-O2 -fdump-tree-profile_estimate -frounding-math" } */


Why the -frounding-math options?


I remember I had some issue with:
  tree r = fold_build2_initializer_loc (UNKNOWN_LOCATION,
MULT_EXPR, t, prob, base);

on targets with a non-IEEE floating point arithmetics (s390?).

 I think test

coverage should handle both that and when that option is not used
if that option makes any difference.


It will eventually pop up if we install new tests w/o rounding math.



Jakub




Martin



I noticed a few minor issues in the hunks below:

--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -12046,7 +12046,8 @@
 when testing pointer or floating-point values.

 This function has the same semantics as @code{__builtin_expect},
 but the caller provides the expected probability that @var{exp} == @var{c}.
 The last argument, @var{probability}, is a floating-point value in the
-range 0.0 to 1.0, inclusive.
+range 0.0 to 1.0, inclusive.  The @var{probability} argument must be
+a compiler time constant.

The term is "compile-time constant" but please see below.

--- a/gcc/predict.c
+++ b/gcc/predict.c
@@ -2467,6 +2467,13 @@
 expr_expected_value_1 (tree type, tree op0, enum tree_code code,
   base = build_real_from_int_cst (t, base);
   tree r = fold_build2_initializer_loc (UNKNOWN_LOCATION,
 MULT_EXPR, t, prob, base);
+  if (TREE_CODE (r) != REAL_CST)
+{
+  error_at (gimple_location (def),
+"probability argument %qE must be a compile "
+"time constant", prob);
+  return NULL;
}

According to GCC coding conventions, when used as an adjective
the term "compile-time" should be hyphenated.  But the term used
in other diagnostics is either "constant integer" or "constant
integer expressions" so I would suggest to use it instead, here
and in the manual.

@@ -2474,6 +2481,11 @@
 expr_expected_value_1 (tree type, tree op0, enum tree_code code,
   *predictor = PRED_BUILTIN_EXPECT_WITH_PROBABILITY;
   *probability = probi;
 }
+  else
+error_at (gimple_location (def),
+  "probability argument %qE must be a in the "
+  "range 0.0 to 1.0", prob);
+

There's a stray 'a' in the text of the error.

But it's not really meaningful to say

  3.14 must be in the range 0.0 to 1.0

because that simply cannot happen.  We could say "argument 2 must
be in the range" but I would instead suggest to rephrase the error
along the same lines as other similar messages GCC already issues:

  "probability %qE is outside the range [0.0, 1.0]"

Martin


Hi Martin.

Thanks for help with the wording. Please take a look at attached patch
candidate.


Looks good!

Thanks
Martin

Re: [PATCH] Verify that last argument of __builtin_expect_with_probability is a real cst (PR c/87811).

2018-11-07 Thread Jeff Law 
On 11/7/18 2:36 AM, Martin Liška wrote:
> On 11/5/18 7:00 PM, Martin Sebor wrote:
>> On 11/01/2018 07:45 AM, Martin Liška wrote:
>>> On 11/1/18 1:15 PM, Jakub Jelinek wrote:
 On Thu, Nov 01, 2018 at 01:09:16PM +0100, Martin Liška wrote:
> -range 0.0 to 1.0, inclusive.
> +range 0.0 to 1.0, inclusive.  The @var{probability} argument must be
> +a compiler time constant.
 When you say must, I think error_at should be used rather than warning_at.
 If others disagree I'm open for leaving it as is.
>>> Error is fine for me as well.
>>>
> @@ -2474,6 +2481,11 @@ expr_expected_value_1 (tree type, tree op0, enum 
> tree_code code,
>    *predictor = PRED_BUILTIN_EXPECT_WITH_PROBABILITY;
>    *probability = probi;
>  }
> +  else
> +  warning_at (gimple_location (def), 0,
> +  "probability argument %qE must be a in the "
> +  "range 0.0 to 1.0", prob);
 Wrong indentation.

 And, no diagnostics for -O0 (which should also be covered by a testcase).
>>> Test for that added.
>>>
> +/* { dg-options "-O2 -fdump-tree-profile_estimate -frounding-math" } */
 Why the -frounding-math options?
>>> I remember I had some issue with:
>>>   tree r = fold_build2_initializer_loc (UNKNOWN_LOCATION,
>>>     MULT_EXPR, t, prob, base);
>>>
>>> on targets with a non-IEEE floating point arithmetics (s390?).
>>>
>>>  I think test
 coverage should handle both that and when that option is not used
 if that option makes any difference.
>>> It will eventually pop up if we install new tests w/o rounding math.
>>>
 Jakub

>>>
>>> Martin
>>>
>> I noticed a few minor issues in the hunks below:
>>
>> --- a/gcc/doc/extend.texi
>> +++ b/gcc/doc/extend.texi
>> @@ -12046,7 +12046,8 @@
>>  when testing pointer or floating-point values.
>>
>>  This function has the same semantics as @code{__builtin_expect},
>>  but the caller provides the expected probability that @var{exp} == @var{c}.
>>  The last argument, @var{probability}, is a floating-point value in the
>> -range 0.0 to 1.0, inclusive.
>> +range 0.0 to 1.0, inclusive.  The @var{probability} argument must be
>> +a compiler time constant.
>>
>> The term is "compile-time constant" but please see below.
>>
>> --- a/gcc/predict.c
>> +++ b/gcc/predict.c
>> @@ -2467,6 +2467,13 @@
>>  expr_expected_value_1 (tree type, tree op0, enum tree_code code,
>>    base = build_real_from_int_cst (t, base);
>>    tree r = fold_build2_initializer_loc (UNKNOWN_LOCATION,
>>  MULT_EXPR, t, prob, base);
>> +  if (TREE_CODE (r) != REAL_CST)
>> +    {
>> +  error_at (gimple_location (def),
>> +    "probability argument %qE must be a compile "
>> +    "time constant", prob);
>> +  return NULL;
>>     }
>>
>> According to GCC coding conventions, when used as an adjective
>> the term "compile-time" should be hyphenated.  But the term used
>> in other diagnostics is either "constant integer" or "constant
>> integer expressions" so I would suggest to use it instead, here
>> and in the manual.
>>
>> @@ -2474,6 +2481,11 @@
>>  expr_expected_value_1 (tree type, tree op0, enum tree_code code,
>>    *predictor = PRED_BUILTIN_EXPECT_WITH_PROBABILITY;
>>    *probability = probi;
>>  }
>> +  else
>> +    error_at (gimple_location (def),
>> +  "probability argument %qE must be a in the "
>> +  "range 0.0 to 1.0", prob);
>> +
>>
>> There's a stray 'a' in the text of the error.
>>
>> But it's not really meaningful to say
>>
>>   3.14 must be in the range 0.0 to 1.0
>>
>> because that simply cannot happen.  We could say "argument 2 must
>> be in the range" but I would instead suggest to rephrase the error
>> along the same lines as other similar messages GCC already issues:
>>
>>   "probability %qE is outside the range [0.0, 1.0]"
>>
>> Martin
> Hi Martin.
> 
> Thanks for help with the wording. Please take a look at attached patch
> candidate.
> 
> Martin
> 
> 
> 0001-Change-wording-of-__builtin_expect_with_probability-.patch
> 
> From 94b61505be171b6b16f7a85c62c722d3c9e13c2f Mon Sep 17 00:00:00 2001
> From: marxin 
> Date: Wed, 7 Nov 2018 10:27:00 +0100
> Subject: [PATCH] Change wording of __builtin_expect_with_probability errors.
> 
> gcc/ChangeLog:
> 
> 2018-11-07  Martin Liska  
> 
>   * doc/extend.texi: Reword.
>   * predict.c (expr_expected_value_1): Likewise.
> 
> gcc/testsuite/ChangeLog:
> 
> 2018-11-07  Martin Liska  
> 
>   * gcc.dg/pr87811.c: Update scanned pattern.
>   * gcc.dg/pr87811-2.c: Likewise.
OK.
jeff

Re: [PATCH] Verify that last argument of __builtin_expect_with_probability is a real cst (PR c/87811).

2018-11-07 Thread Martin Liška 
On 11/5/18 7:00 PM, Martin Sebor wrote:
> On 11/01/2018 07:45 AM, Martin Liška wrote:
>> On 11/1/18 1:15 PM, Jakub Jelinek wrote:
>>> On Thu, Nov 01, 2018 at 01:09:16PM +0100, Martin Liška wrote:
 -range 0.0 to 1.0, inclusive.
 +range 0.0 to 1.0, inclusive.  The @var{probability} argument must be
 +a compiler time constant.
>>>
>>> When you say must, I think error_at should be used rather than warning_at.
>>> If others disagree I'm open for leaving it as is.
>>
>> Error is fine for me as well.
>>
>>>
 @@ -2474,6 +2481,11 @@ expr_expected_value_1 (tree type, tree op0, enum 
 tree_code code,
    *predictor = PRED_BUILTIN_EXPECT_WITH_PROBABILITY;
    *probability = probi;
  }
 +  else
 +  warning_at (gimple_location (def), 0,
 +  "probability argument %qE must be a in the "
 +  "range 0.0 to 1.0", prob);
>>>
>>> Wrong indentation.
>>>
>>> And, no diagnostics for -O0 (which should also be covered by a testcase).
>>
>> Test for that added.
>>
>>>
 +/* { dg-options "-O2 -fdump-tree-profile_estimate -frounding-math" } */
>>>
>>> Why the -frounding-math options?
>>
>> I remember I had some issue with:
>>   tree r = fold_build2_initializer_loc (UNKNOWN_LOCATION,
>>     MULT_EXPR, t, prob, base);
>>
>> on targets with a non-IEEE floating point arithmetics (s390?).
>>
>>  I think test
>>> coverage should handle both that and when that option is not used
>>> if that option makes any difference.
>>
>> It will eventually pop up if we install new tests w/o rounding math.
>>
>>>
>>> Jakub
>>>
>>
>>
>> Martin
>>
> 
> I noticed a few minor issues in the hunks below:
> 
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -12046,7 +12046,8 @@
>  when testing pointer or floating-point values.
> 
>  This function has the same semantics as @code{__builtin_expect},
>  but the caller provides the expected probability that @var{exp} == @var{c}.
>  The last argument, @var{probability}, is a floating-point value in the
> -range 0.0 to 1.0, inclusive.
> +range 0.0 to 1.0, inclusive.  The @var{probability} argument must be
> +a compiler time constant.
> 
> The term is "compile-time constant" but please see below.
> 
> --- a/gcc/predict.c
> +++ b/gcc/predict.c
> @@ -2467,6 +2467,13 @@
>  expr_expected_value_1 (tree type, tree op0, enum tree_code code,
>    base = build_real_from_int_cst (t, base);
>    tree r = fold_build2_initializer_loc (UNKNOWN_LOCATION,
>  MULT_EXPR, t, prob, base);
> +  if (TREE_CODE (r) != REAL_CST)
> +    {
> +  error_at (gimple_location (def),
> +    "probability argument %qE must be a compile "
> +    "time constant", prob);
> +  return NULL;
>     }
> 
> According to GCC coding conventions, when used as an adjective
> the term "compile-time" should be hyphenated.  But the term used
> in other diagnostics is either "constant integer" or "constant
> integer expressions" so I would suggest to use it instead, here
> and in the manual.
> 
> @@ -2474,6 +2481,11 @@
>  expr_expected_value_1 (tree type, tree op0, enum tree_code code,
>    *predictor = PRED_BUILTIN_EXPECT_WITH_PROBABILITY;
>    *probability = probi;
>  }
> +  else
> +    error_at (gimple_location (def),
> +  "probability argument %qE must be a in the "
> +  "range 0.0 to 1.0", prob);
> +
> 
> There's a stray 'a' in the text of the error.
> 
> But it's not really meaningful to say
> 
>   3.14 must be in the range 0.0 to 1.0
> 
> because that simply cannot happen.  We could say "argument 2 must
> be in the range" but I would instead suggest to rephrase the error
> along the same lines as other similar messages GCC already issues:
> 
>   "probability %qE is outside the range [0.0, 1.0]"
> 
> Martin

Hi Martin.

Thanks for help with the wording. Please take a look at attached patch
candidate.

Martin
>From 94b61505be171b6b16f7a85c62c722d3c9e13c2f Mon Sep 17 00:00:00 2001
From: marxin 
Date: Wed, 7 Nov 2018 10:27:00 +0100
Subject: [PATCH] Change wording of __builtin_expect_with_probability errors.

gcc/ChangeLog:

2018-11-07  Martin Liska  

	* doc/extend.texi: Reword.
	* predict.c (expr_expected_value_1): Likewise.

gcc/testsuite/ChangeLog:

2018-11-07  Martin Liska  

	* gcc.dg/pr87811.c: Update scanned pattern.
	* gcc.dg/pr87811-2.c: Likewise.
---
 gcc/doc/extend.texi  | 2 +-
 gcc/predict.c| 8 
 gcc/testsuite/gcc.dg/pr87811-2.c | 2 +-
 gcc/testsuite/gcc.dg/pr87811.c   | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 7d16129..d6802ad3467 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -12047,7 +12047,7 @@ This function has the same semantics as

Re: [PATCH] Verify that last argument of __builtin_expect_with_probability is a real cst (PR c/87811).

2018-11-05 Thread Martin Sebor 

On 11/01/2018 07:45 AM, Martin Liška wrote:

On 11/1/18 1:15 PM, Jakub Jelinek wrote:

On Thu, Nov 01, 2018 at 01:09:16PM +0100, Martin Liška wrote:

-range 0.0 to 1.0, inclusive.
+range 0.0 to 1.0, inclusive.  The @var{probability} argument must be
+a compiler time constant.


When you say must, I think error_at should be used rather than warning_at.
If others disagree I'm open for leaving it as is.


Error is fine for me as well.




@@ -2474,6 +2481,11 @@ expr_expected_value_1 (tree type, tree op0, enum 
tree_code code,
  *predictor = PRED_BUILTIN_EXPECT_WITH_PROBABILITY;
  *probability = probi;
}
+ else
+ warning_at (gimple_location (def), 0,
+ "probability argument %qE must be a in the "
+ "range 0.0 to 1.0", prob);


Wrong indentation.

And, no diagnostics for -O0 (which should also be covered by a testcase).


Test for that added.




+/* { dg-options "-O2 -fdump-tree-profile_estimate -frounding-math" } */


Why the -frounding-math options?


I remember I had some issue with:
  tree r = fold_build2_initializer_loc (UNKNOWN_LOCATION,
MULT_EXPR, t, prob, 
base);

on targets with a non-IEEE floating point arithmetics (s390?).

 I think test

coverage should handle both that and when that option is not used
if that option makes any difference.


It will eventually pop up if we install new tests w/o rounding math.



Jakub




Martin



I noticed a few minor issues in the hunks below:

--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -12046,7 +12046,8 @@
 when testing pointer or floating-point values.

 This function has the same semantics as @code{__builtin_expect},
 but the caller provides the expected probability that @var{exp} == 
@var{c}.

 The last argument, @var{probability}, is a floating-point value in the
-range 0.0 to 1.0, inclusive.
+range 0.0 to 1.0, inclusive.  The @var{probability} argument must be
+a compiler time constant.

The term is "compile-time constant" but please see below.

--- a/gcc/predict.c
+++ b/gcc/predict.c
@@ -2467,6 +2467,13 @@
 expr_expected_value_1 (tree type, tree op0, enum tree_code code,
  base = build_real_from_int_cst (t, base);
  tree r = fold_build2_initializer_loc (UNKNOWN_LOCATION,
MULT_EXPR, t, prob, 
base);
+ if (TREE_CODE (r) != REAL_CST)
+   {
+ error_at (gimple_location (def),
+   "probability argument %qE must be a compile "
+   "time constant", prob);
+ return NULL;
}

According to GCC coding conventions, when used as an adjective
the term "compile-time" should be hyphenated.  But the term used
in other diagnostics is either "constant integer" or "constant
integer expressions" so I would suggest to use it instead, here
and in the manual.

@@ -2474,6 +2481,11 @@
 expr_expected_value_1 (tree type, tree op0, enum tree_code code,
  *predictor = PRED_BUILTIN_EXPECT_WITH_PROBABILITY;
  *probability = probi;
}
+ else
+   error_at (gimple_location (def),
+ "probability argument %qE must be a in the "
+ "range 0.0 to 1.0", prob);
+

There's a stray 'a' in the text of the error.

But it's not really meaningful to say

  3.14 must be in the range 0.0 to 1.0

because that simply cannot happen.  We could say "argument 2 must
be in the range" but I would instead suggest to rephrase the error
along the same lines as other similar messages GCC already issues:

  "probability %qE is outside the range [0.0, 1.0]"

Martin

Re: [PATCH] Verify that last argument of __builtin_expect_with_probability is a real cst (PR c/87811).

2018-11-02 Thread Jeff Law 
On 11/1/18 7:45 AM, Martin Liška wrote:
> On 11/1/18 1:15 PM, Jakub Jelinek wrote:
>> On Thu, Nov 01, 2018 at 01:09:16PM +0100, Martin Liška wrote:
>>> -range 0.0 to 1.0, inclusive.
>>> +range 0.0 to 1.0, inclusive.  The @var{probability} argument must be
>>> +a compiler time constant.
>> When you say must, I think error_at should be used rather than warning_at.
>> If others disagree I'm open for leaving it as is.
> Error is fine for me as well.
> 
>>> @@ -2474,6 +2481,11 @@ expr_expected_value_1 (tree type, tree op0, enum 
>>> tree_code code,
>>>   *predictor = PRED_BUILTIN_EXPECT_WITH_PROBABILITY;
>>>   *probability = probi;
>>> }
>>> + else
>>> + warning_at (gimple_location (def), 0,
>>> + "probability argument %qE must be a in the "
>>> + "range 0.0 to 1.0", prob);
>> Wrong indentation.
>>
>> And, no diagnostics for -O0 (which should also be covered by a testcase).
> Test for that added.
> 
>>> +/* { dg-options "-O2 -fdump-tree-profile_estimate -frounding-math" } */
>> Why the -frounding-math options? 
> I remember I had some issue with:
> tree r = fold_build2_initializer_loc (UNKNOWN_LOCATION,
>   MULT_EXPR, t, prob, 
> base);
> 
> on targets with a non-IEEE floating point arithmetics (s390?).
> 
>  I think test
>> coverage should handle both that and when that option is not used
>> if that option makes any difference.
> It will eventually pop up if we install new tests w/o rounding math.
> 
>>  Jakub
>>
> 
> Martin
> 
> 
> 0001-Verify-that-last-argument-of-__builtin_expect_with_p.patch
> 
> From 7e0834a2ebe1a3fb83304197a843dca63332bc78 Mon Sep 17 00:00:00 2001
> From: marxin 
> Date: Tue, 30 Oct 2018 14:01:59 +0100
> Subject: [PATCH] Verify that last argument of
>  __builtin_expect_with_probability is a real cst (PR c/87811).
> 
> gcc/ChangeLog:
> 
> 2018-10-30  Martin Liska  
> 
>   PR c/87811
>   * predict.c (expr_expected_value_1): Verify
>   that last argument is a real constants and emit
>   error.
> 
> gcc/testsuite/ChangeLog:
> 
> 2018-10-30  Martin Liska  
> 
>   PR c/87811
>   * gcc.dg/pr87811.c: New test.
>   * gcc.dg/pr87811-2.c: Likewise.
>   * gcc.dg/pr87811-3.c: Likewise.
> 
> gcc/ChangeLog:
> 
> 2018-10-30  Martin Liska  
> 
>   * doc/extend.texi: Update constrain about the last argument
>   of __builtin_expect_with_probability.
> ---
>  gcc/doc/extend.texi  |  3 ++-
>  gcc/predict.c| 12 
>  gcc/testsuite/gcc.dg/pr87811-2.c | 13 +
>  gcc/testsuite/gcc.dg/pr87811-3.c | 11 +++
>  gcc/testsuite/gcc.dg/pr87811.c   | 13 +
>  5 files changed, 51 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.dg/pr87811-2.c
>  create mode 100644 gcc/testsuite/gcc.dg/pr87811-3.c
>  create mode 100644 gcc/testsuite/gcc.dg/pr87811.c
OK
jeff

Re: [PATCH] Verify that last argument of __builtin_expect_with_probability is a real cst (PR c/87811).

2018-11-01 Thread Martin Liška 
On 11/1/18 1:15 PM, Jakub Jelinek wrote:
> On Thu, Nov 01, 2018 at 01:09:16PM +0100, Martin Liška wrote:
>> -range 0.0 to 1.0, inclusive.
>> +range 0.0 to 1.0, inclusive.  The @var{probability} argument must be
>> +a compiler time constant.
> 
> When you say must, I think error_at should be used rather than warning_at.
> If others disagree I'm open for leaving it as is.

Error is fine for me as well.

> 
>> @@ -2474,6 +2481,11 @@ expr_expected_value_1 (tree type, tree op0, enum 
>> tree_code code,
>>*predictor = PRED_BUILTIN_EXPECT_WITH_PROBABILITY;
>>*probability = probi;
>>  }
>> +  else
>> +  warning_at (gimple_location (def), 0,
>> +  "probability argument %qE must be a in the "
>> +  "range 0.0 to 1.0", prob);
> 
> Wrong indentation.
> 
> And, no diagnostics for -O0 (which should also be covered by a testcase).

Test for that added.

> 
>> +/* { dg-options "-O2 -fdump-tree-profile_estimate -frounding-math" } */
> 
> Why the -frounding-math options? 

I remember I had some issue with:
  tree r = fold_build2_initializer_loc (UNKNOWN_LOCATION,
MULT_EXPR, t, prob, 
base);

on targets with a non-IEEE floating point arithmetics (s390?).

 I think test
> coverage should handle both that and when that option is not used
> if that option makes any difference.

It will eventually pop up if we install new tests w/o rounding math.

> 
>   Jakub
> 


Martin
>From 7e0834a2ebe1a3fb83304197a843dca63332bc78 Mon Sep 17 00:00:00 2001
From: marxin 
Date: Tue, 30 Oct 2018 14:01:59 +0100
Subject: [PATCH] Verify that last argument of
 __builtin_expect_with_probability is a real cst (PR c/87811).

gcc/ChangeLog:

2018-10-30  Martin Liska  

	PR c/87811
	* predict.c (expr_expected_value_1): Verify
	that last argument is a real constants and emit
	error.

gcc/testsuite/ChangeLog:

2018-10-30  Martin Liska  

	PR c/87811
	* gcc.dg/pr87811.c: New test.
	* gcc.dg/pr87811-2.c: Likewise.
	* gcc.dg/pr87811-3.c: Likewise.

gcc/ChangeLog:

2018-10-30  Martin Liska  

	* doc/extend.texi: Update constrain about the last argument
	of __builtin_expect_with_probability.
---
 gcc/doc/extend.texi  |  3 ++-
 gcc/predict.c| 12 
 gcc/testsuite/gcc.dg/pr87811-2.c | 13 +
 gcc/testsuite/gcc.dg/pr87811-3.c | 11 +++
 gcc/testsuite/gcc.dg/pr87811.c   | 13 +
 5 files changed, 51 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr87811-2.c
 create mode 100644 gcc/testsuite/gcc.dg/pr87811-3.c
 create mode 100644 gcc/testsuite/gcc.dg/pr87811.c

diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index e2b9ee11a54..3dabd486dbf 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -12046,7 +12046,8 @@ when testing pointer or floating-point values.
 This function has the same semantics as @code{__builtin_expect},
 but the caller provides the expected probability that @var{exp} == @var{c}.
 The last argument, @var{probability}, is a floating-point value in the
-range 0.0 to 1.0, inclusive.
+range 0.0 to 1.0, inclusive.  The @var{probability} argument must be
+a compiler time constant.
 @end deftypefn
 
 @deftypefn {Built-in Function} void __builtin_trap (void)
diff --git a/gcc/predict.c b/gcc/predict.c
index ab2dc8ed031..80a8d6846f7 100644
--- a/gcc/predict.c
+++ b/gcc/predict.c
@@ -2467,6 +2467,13 @@ expr_expected_value_1 (tree type, tree op0, enum tree_code code,
 		  base = build_real_from_int_cst (t, base);
 		  tree r = fold_build2_initializer_loc (UNKNOWN_LOCATION,
 			MULT_EXPR, t, prob, base);
+		  if (TREE_CODE (r) != REAL_CST)
+		{
+		  error_at (gimple_location (def),
+"probability argument %qE must be a compile "
+"time constant", prob);
+		  return NULL;
+		}
 		  HOST_WIDE_INT probi
 		= real_to_integer (TREE_REAL_CST_PTR (r));
 		  if (probi >= 0 && probi <= REG_BR_PROB_BASE)
@@ -2474,6 +2481,11 @@ expr_expected_value_1 (tree type, tree op0, enum tree_code code,
 		  *predictor = PRED_BUILTIN_EXPECT_WITH_PROBABILITY;
 		  *probability = probi;
 		}
+		  else
+		error_at (gimple_location (def),
+			  "probability argument %qE must be a in the "
+			  "range 0.0 to 1.0", prob);
+
 		  return gimple_call_arg (def, 1);
 		}
 
diff --git a/gcc/testsuite/gcc.dg/pr87811-2.c b/gcc/testsuite/gcc.dg/pr87811-2.c
new file mode 100644
index 000..aa30ddff47b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr87811-2.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-profile_estimate" } */
+
+void bar (void);
+
+void
+foo (int i)
+{
+  if (__builtin_expect_with_probability (i, 0, 2.0f)) /* { dg-error "probability argument .* must be a in the range 0\\\.0 to 1\\\.0" } */
+bar ();
+}
+
+/* { dg-final { scan-tree-dump-not "__builtin_expect_with_probability

Re: [PATCH] Verify that last argument of __builtin_expect_with_probability is a real cst (PR c/87811).

2018-11-01 Thread Jakub Jelinek 
On Thu, Nov 01, 2018 at 01:09:16PM +0100, Martin Liška wrote:
> -range 0.0 to 1.0, inclusive.
> +range 0.0 to 1.0, inclusive.  The @var{probability} argument must be
> +a compiler time constant.

When you say must, I think error_at should be used rather than warning_at.
If others disagree I'm open for leaving it as is.

> @@ -2474,6 +2481,11 @@ expr_expected_value_1 (tree type, tree op0, enum 
> tree_code code,
> *predictor = PRED_BUILTIN_EXPECT_WITH_PROBABILITY;
> *probability = probi;
>   }
> +   else
> +   warning_at (gimple_location (def), 0,
> +   "probability argument %qE must be a in the "
> +   "range 0.0 to 1.0", prob);

Wrong indentation.

And, no diagnostics for -O0 (which should also be covered by a testcase).

> +/* { dg-options "-O2 -fdump-tree-profile_estimate -frounding-math" } */

Why the -frounding-math options?  I think test
coverage should handle both that and when that option is not used
if that option makes any difference.

Jakub

Re: [PATCH] Verify that last argument of __builtin_expect_with_probability is a real cst (PR c/87811).

2018-11-01 Thread Martin Liška 
On 10/31/18 11:17 AM, Jakub Jelinek wrote:
> On Wed, Oct 31, 2018 at 11:04:32AM +0100, Martin Liška wrote:
>> Hi.
>>
>> As Jakub pointed out we should not ICE when last argument
>> of __builtin_expect_with_probability is not a real cst.
>> Plus I documented the behavior.
> 
> That is not what you've implemented.  The documentation says that
> it must be a compile time constant, but nothing enforces it, just silently
> ignores it.  So, either diagnose it at the time when
> __builtin_expect_with_probability is removed from the IL, i.e.
> the code you've touched? and expand_builtin_expect_with_probability
> and the argument would be really a late constant (constant after
> inlining and optimizations), or diagnose it in fold_builtin_expect
> and replace with the argument at that point.  And, please diagnose also the
> cases where the probability is not >= 0.0 and <= 1.0.

I provided 2 warnings for both of these situations.

>>  * doc/extend.texi: Update constrain about the last arguemnt
> 
> s/arguemnt/argument/

Fixed.

New patch survives regression tests on x86_64-linux-gnu.

Martin

> 
>   Jakub
> 

>From bae9e3abeeea9b4e2dcde8c1581efaaffd800899 Mon Sep 17 00:00:00 2001
From: marxin 
Date: Tue, 30 Oct 2018 14:01:59 +0100
Subject: [PATCH] Verify that last argument of
 __builtin_expect_with_probability is a real cst (PR c/87811).

gcc/ChangeLog:

2018-10-30  Martin Liska  

	PR c/87811
	* predict.c (expr_expected_value_1): Verify
	that last argument is a real constants.

gcc/testsuite/ChangeLog:

2018-10-30  Martin Liska  

	PR c/87811
	* gcc.dg/pr87811.c: New test.
	* gcc.dg/pr87811-2.c: Likewise.

gcc/ChangeLog:

2018-10-30  Martin Liska  

	* doc/extend.texi: Update constrain about the last argument
	of __builtin_expect_with_probability.
---
 gcc/doc/extend.texi  |  3 ++-
 gcc/predict.c| 12 
 gcc/testsuite/gcc.dg/pr87811-2.c | 13 +
 gcc/testsuite/gcc.dg/pr87811.c   | 13 +
 4 files changed, 40 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr87811-2.c
 create mode 100644 gcc/testsuite/gcc.dg/pr87811.c

diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index e2b9ee11a54..3dabd486dbf 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -12046,7 +12046,8 @@ when testing pointer or floating-point values.
 This function has the same semantics as @code{__builtin_expect},
 but the caller provides the expected probability that @var{exp} == @var{c}.
 The last argument, @var{probability}, is a floating-point value in the
-range 0.0 to 1.0, inclusive.
+range 0.0 to 1.0, inclusive.  The @var{probability} argument must be
+a compiler time constant.
 @end deftypefn
 
 @deftypefn {Built-in Function} void __builtin_trap (void)
diff --git a/gcc/predict.c b/gcc/predict.c
index ab2dc8ed031..43621653fee 100644
--- a/gcc/predict.c
+++ b/gcc/predict.c
@@ -2467,6 +2467,13 @@ expr_expected_value_1 (tree type, tree op0, enum tree_code code,
 		  base = build_real_from_int_cst (t, base);
 		  tree r = fold_build2_initializer_loc (UNKNOWN_LOCATION,
 			MULT_EXPR, t, prob, base);
+		  if (TREE_CODE (r) != REAL_CST)
+		{
+		  warning_at (gimple_location (def), 0,
+  "probability argument %qE must be a compile "
+  "time constant", prob);
+		  return NULL;
+		}
 		  HOST_WIDE_INT probi
 		= real_to_integer (TREE_REAL_CST_PTR (r));
 		  if (probi >= 0 && probi <= REG_BR_PROB_BASE)
@@ -2474,6 +2481,11 @@ expr_expected_value_1 (tree type, tree op0, enum tree_code code,
 		  *predictor = PRED_BUILTIN_EXPECT_WITH_PROBABILITY;
 		  *probability = probi;
 		}
+		  else
+		  warning_at (gimple_location (def), 0,
+  "probability argument %qE must be a in the "
+  "range 0.0 to 1.0", prob);
+
 		  return gimple_call_arg (def, 1);
 		}
 
diff --git a/gcc/testsuite/gcc.dg/pr87811-2.c b/gcc/testsuite/gcc.dg/pr87811-2.c
new file mode 100644
index 000..27d910c9305
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr87811-2.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-profile_estimate -frounding-math" } */
+
+void bar (void);
+
+void
+foo (int i)
+{
+  if (__builtin_expect_with_probability (i, 0, 2.0f)) /* { dg-warning "probability argument .* must be a in the range 0\\\.0 to 1\\\.0" } */
+bar ();
+}
+
+/* { dg-final { scan-tree-dump-not "__builtin_expect_with_probability heuristics of edge" "profile_estimate"} } */
diff --git a/gcc/testsuite/gcc.dg/pr87811.c b/gcc/testsuite/gcc.dg/pr87811.c
new file mode 100644
index 000..18e5c0d97db
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr87811.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-profile_estimate -frounding-math" } */
+
+void bar (void);
+
+void
+foo (int i, double d)
+{
+  if (__builtin_expect_with_probability (i, 0, d)) /* { dg-warning "probability argument .d. must be a compile time constant" } */
+bar ();
+}
+
+/* { dg-final { scan-tree-dump-not

Re: [PATCH] Verify that last argument of __builtin_expect_with_probability is a real cst (PR c/87811).

2018-10-31 Thread Jakub Jelinek 
On Wed, Oct 31, 2018 at 11:04:32AM +0100, Martin Liška wrote:
> Hi.
> 
> As Jakub pointed out we should not ICE when last argument
> of __builtin_expect_with_probability is not a real cst.
> Plus I documented the behavior.

That is not what you've implemented.  The documentation says that
it must be a compile time constant, but nothing enforces it, just silently
ignores it.  So, either diagnose it at the time when
__builtin_expect_with_probability is removed from the IL, i.e.
the code you've touched? and expand_builtin_expect_with_probability
and the argument would be really a late constant (constant after
inlining and optimizations), or diagnose it in fold_builtin_expect
and replace with the argument at that point.  And, please diagnose also the
cases where the probability is not >= 0.0 and <= 1.0.
>   * doc/extend.texi: Update constrain about the last arguemnt

s/arguemnt/argument/

Jakub