| Message ID | 3bf1c657-c5cd-5e87-5f36-064abf1b5fc3@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Verify that last argument of __builtin_expect_with_probability is a real cst (PR c/87811). | expand |
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
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 <mliska@suse.cz> 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 <mliska@suse.cz> 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 <mliska@suse.cz> PR c/87811 * gcc.dg/pr87811.c: New test. * gcc.dg/pr87811-2.c: Likewise. gcc/ChangeLog: 2018-10-30 Martin Liska <mliska@suse.cz> * 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 00000000000..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 00000000000..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 "__builtin_expect_with_probability heuristics of edge" "profile_estimate"} } */
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
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 <mliska@suse.cz> 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 <mliska@suse.cz> 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 <mliska@suse.cz> 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 <mliska@suse.cz> * 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 00000000000..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 heuristics of edge" "profile_estimate"} } */ diff --git a/gcc/testsuite/gcc.dg/pr87811-3.c b/gcc/testsuite/gcc.dg/pr87811-3.c new file mode 100644 index 00000000000..720d154a8d4 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr87811-3.c @@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-options "-fdump-tree-profile_estimate" } */ + +void bar (void); + +void +foo (int i) +{ + if (__builtin_expect_with_probability (i, 0, 2.0f)) + bar (); +} diff --git a/gcc/testsuite/gcc.dg/pr87811.c b/gcc/testsuite/gcc.dg/pr87811.c new file mode 100644 index 00000000000..9045c8ea957 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr87811.c @@ -0,0 +1,13 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-profile_estimate" } */ + +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" } */ + bar (); +} + +/* { dg-final { scan-tree-dump-not "__builtin_expect_with_probability heuristics of edge" "profile_estimate"} } */
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 <mliska@suse.cz> > 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 <mliska@suse.cz> > > 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 <mliska@suse.cz> > > 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 <mliska@suse.cz> > > * 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
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
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 <mliska@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 <mliska@suse.cz> * doc/extend.texi: Reword. * predict.c (expr_expected_value_1): Likewise. gcc/testsuite/ChangeLog: 2018-11-07 Martin Liska <mliska@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 (); }
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 <mliska@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 <mliska@suse.cz> > > * doc/extend.texi: Reword. > * predict.c (expr_expected_value_1): Likewise. > > gcc/testsuite/ChangeLog: > > 2018-11-07 Martin Liska <mliska@suse.cz> > > * gcc.dg/pr87811.c: Update scanned pattern. > * gcc.dg/pr87811-2.c: Likewise. OK. jeff
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
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index 4dbb2da39e4..0033bf12e62 100644 --- a/gcc/doc/extend.texi +++ b/gcc/doc/extend.texi @@ -12030,7 +12030,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..bb116073e43 100644 --- a/gcc/predict.c +++ b/gcc/predict.c @@ -2467,6 +2467,8 @@ 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) + return NULL; HOST_WIDE_INT probi = real_to_integer (TREE_REAL_CST_PTR (r)); if (probi >= 0 && probi <= REG_BR_PROB_BASE) diff --git a/gcc/testsuite/gcc.dg/pr87811.c b/gcc/testsuite/gcc.dg/pr87811.c new file mode 100644 index 00000000000..f4ee21c4847 --- /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)) + bar (); +} + +/* { dg-final { scan-tree-dump-not "__builtin_expect_with_probability heuristics of edge" "profile_estimate"} } */