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

[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