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 <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.
OK.
jeff
