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 <mli...@suse.cz>
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 <mli...@suse.cz>
* doc/extend.texi: Reword.
* predict.c (expr_expected_value_1): Likewise.
gcc/testsuite/ChangeLog:
2018-11-07 Martin Liska <mli...@suse.cz>
* 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 7d144446129..d6802ad3467 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -12047,7 +12047,7 @@ 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. The @var{probability} argument must be
-a compiler time constant.
+constant floating-point expression.
@end deftypefn
@deftypefn {Built-in Function} void __builtin_trap (void)
diff --git a/gcc/predict.c b/gcc/predict.c
index 80a8d6846f7..8482737ab27 100644
--- a/gcc/predict.c
+++ b/gcc/predict.c
@@ -2470,8 +2470,8 @@ expr_expected_value_1 (tree type, tree op0, enum tree_code code,
if (TREE_CODE (r) != REAL_CST)
{
error_at (gimple_location (def),
- "probability argument %qE must be a compile "
- "time constant", prob);
+ "probability %qE must be "
+ "constant floating-point expression", prob);
return NULL;
}
HOST_WIDE_INT probi
@@ -2483,8 +2483,8 @@ expr_expected_value_1 (tree type, tree op0, enum tree_code code,
}
else
error_at (gimple_location (def),
- "probability argument %qE must be a in the "
- "range 0.0 to 1.0", prob);
+ "probability %qE is outside "
+ "the range [0.0, 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
index aa30ddff47b..8b0818756e2 100644
--- a/gcc/testsuite/gcc.dg/pr87811-2.c
+++ b/gcc/testsuite/gcc.dg/pr87811-2.c
@@ -6,7 +6,7 @@ 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" } */
+ if (__builtin_expect_with_probability (i, 0, 2.0f)) /* { dg-error "probability .* is outside the range \\\[0\\\.0, 1\\\.0\\\]" } */
bar ();
}
diff --git a/gcc/testsuite/gcc.dg/pr87811.c b/gcc/testsuite/gcc.dg/pr87811.c
index 9045c8ea957..110b3835975 100644
--- a/gcc/testsuite/gcc.dg/pr87811.c
+++ b/gcc/testsuite/gcc.dg/pr87811.c
@@ -6,7 +6,7 @@ void bar (void);
void
foo (int i, double d)
{
- if (__builtin_expect_with_probability (i, 0, d)) /* { dg-error "probability argument .d. must be a compile time constant" } */
+ if (__builtin_expect_with_probability (i, 0, d)) /* { dg-error "probability .d. must be constant floating-point expression" } */
bar ();
}
--
2.19.1
