This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 1/3] Come up with selftests for predict.c.


On 06/06/2017 12:44 PM, David Malcolm wrote:
> On Tue, 2017-06-06 at 10:55 +0200, marxin wrote:
> 
> Some minor nits below.
> 
>> gcc/ChangeLog:
>>
>> 2017-05-26  Martin Liska  <mliska@suse.cz>
>>
>> 	* predict.c (struct branch_predictor): New struct.
>> 	(test_prediction_value_range): New test.
>> 	(predict_tests_c_tests): New function.
>> 	* selftest-run-tests.c (selftest::run_tests): Run the function.
>> 	* selftest.h: Declare new tests.
>> ---
>>  gcc/predict.c            | 39
>> +++++++++++++++++++++++++++++++++++++++
>>  gcc/selftest-run-tests.c |  1 +
>>  gcc/selftest.h           |  1 +
>>  3 files changed, 41 insertions(+)
>>
>> diff --git a/gcc/predict.c b/gcc/predict.c
>> index ac35fa41129..ea445e94e46 100644
>> --- a/gcc/predict.c
>> +++ b/gcc/predict.c
>> @@ -57,6 +57,7 @@ along with GCC; see the file COPYING3.  If not see
>>  #include "tree-scalar-evolution.h"
>>  #include "ipa-utils.h"
>>  #include "gimple-pretty-print.h"
>> +#include "selftest.h"
>>  
>>  /* Enum with reasons why a predictor is ignored.  */
>>  
>> @@ -3803,3 +3804,41 @@ force_edge_cold (edge e, bool impossible)
>>  		 impossible ? "impossible" : "cold");
>>      }
>>  }
>> +
>> +#if CHECKING_P
>> +
>> +namespace selftest {
>> +
>> +/* Test that value range of predictor values defined in predict.def
>> is
>> +   within range [51, 100].  */
>> +
>> +struct branch_predictor
>> +{
>> +  const char *name;
>> +  unsigned probability;
>> +};
>> +
>> +#define DEF_PREDICTOR(ENUM, NAME, HITRATE, FLAGS) { NAME, HITRATE },
>> +
>> +static void
>> +test_prediction_value_range ()
>> +{
>> +  branch_predictor predictors[] = {
>> +#include "predict.def"
>> +    {NULL, -1}
>> +  };
>> +
>> +  for (unsigned i = 0; predictors[i].name != NULL; i++)
>> +    ASSERT_TRUE (100 * predictors[i].probability / REG_BR_PROB_BASE
>>> 50);
> 
> Minor nit: should there be a #undef DEF_PREDICTOR after the #include?

Hello.

Yes, that should be undefined.

> 
> Minor nits: the comment says it verifies that things are in the range
> 51, 100.  Should it be >= 51 rather than > 50?  Should there be a test
> that it's <= 100?  (I'm not familiar with the predict code, I just
> noticed this when reading the comment).

Should be in range (50,100], fixed accordingly.

> 
>> +}
>> +
>> +/* Run all of the selfests within this file.  */
>> +
>> +void
>> +predict_tests_c_tests ()
>> +{
> 
> Minor nit: to follow the pattern used in the other selftests, we've
> been naming these "run all selftests within this file" functions after
> the filename.  Given this is in predict.c, this should be
> "predict_c_tests ()" to follow the pattern.

I followed your naming scheme.

Thanks for review,
Martin

> 
>> +  test_prediction_value_range ();
>> +}
>> +
>> +} // namespace selftest
>> +#endif /* CHECKING_P.  */
>> diff --git a/gcc/selftest-run-tests.c b/gcc/selftest-run-tests.c
>> index f62bc72b072..af511a47681 100644
>> --- a/gcc/selftest-run-tests.c
>> +++ b/gcc/selftest-run-tests.c
>> @@ -92,6 +92,7 @@ selftest::run_tests ()
>>      targetm.run_target_selftests ();
>>  
>>    store_merging_c_tests ();
>> +  predict_tests_c_tests ();
> 
> ...and here.
> 
> 
>>    /* Run any lang-specific selftests.  */
>>    lang_hooks.run_lang_selftests ();
>> diff --git a/gcc/selftest.h b/gcc/selftest.h
>> index dad53e9fe09..e84b309359d 100644
>> --- a/gcc/selftest.h
>> +++ b/gcc/selftest.h
>> @@ -196,6 +196,7 @@ extern void tree_c_tests ();
>>  extern void tree_cfg_c_tests ();
>>  extern void vec_c_tests ();
>>  extern void wide_int_cc_tests ();
>> +extern void predict_tests_c_tests ();
> 
> (etc)
>  
>>  extern int num_passes;
> 
> Thanks; hope this is constructive.
> Dave
> 

>From 05d47c50fbc72fcf4f91e1cd923bf9453e13eddd Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Tue, 6 Jun 2017 10:55:10 +0200
Subject: [PATCH] Come up with selftests for predict.c.

gcc/ChangeLog:

2017-05-26  Martin Liska  <mliska@suse.cz>

	* predict.c (struct branch_predictor): New struct.
	(test_prediction_value_range): New test.
	(predict_c_tests): New function.
	* selftest-run-tests.c (selftest::run_tests): Run the function.
	* selftest.h: Declare new tests.
---
 gcc/predict.c            | 44 ++++++++++++++++++++++++++++++++++++++++++++
 gcc/selftest-run-tests.c |  1 +
 gcc/selftest.h           |  1 +
 3 files changed, 46 insertions(+)

diff --git a/gcc/predict.c b/gcc/predict.c
index ac35fa41129..5685012d491 100644
--- a/gcc/predict.c
+++ b/gcc/predict.c
@@ -57,6 +57,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-scalar-evolution.h"
 #include "ipa-utils.h"
 #include "gimple-pretty-print.h"
+#include "selftest.h"
 
 /* Enum with reasons why a predictor is ignored.  */
 
@@ -3803,3 +3804,46 @@ force_edge_cold (edge e, bool impossible)
 		 impossible ? "impossible" : "cold");
     }
 }
+
+#if CHECKING_P
+
+namespace selftest {
+
+/* Test that value range of predictor values defined in predict.def is
+   within range (50, 100].  */
+
+struct branch_predictor
+{
+  const char *name;
+  unsigned probability;
+};
+
+#define DEF_PREDICTOR(ENUM, NAME, HITRATE, FLAGS) { NAME, HITRATE },
+
+static void
+test_prediction_value_range ()
+{
+  branch_predictor predictors[] = {
+#include "predict.def"
+    {NULL, -1}
+  };
+
+  for (unsigned i = 0; predictors[i].name != NULL; i++)
+    {
+      unsigned p = 100 * predictors[i].probability / REG_BR_PROB_BASE;
+      ASSERT_TRUE (p > 50 && p <= 100);
+    }
+}
+
+#undef DEF_PREDICTOR
+
+/* Run all of the selfests within this file.  */
+
+void
+predict_c_tests ()
+{
+  test_prediction_value_range ();
+}
+
+} // namespace selftest
+#endif /* CHECKING_P.  */
diff --git a/gcc/selftest-run-tests.c b/gcc/selftest-run-tests.c
index f62bc72b072..30e476d14c5 100644
--- a/gcc/selftest-run-tests.c
+++ b/gcc/selftest-run-tests.c
@@ -92,6 +92,7 @@ selftest::run_tests ()
     targetm.run_target_selftests ();
 
   store_merging_c_tests ();
+  predict_c_tests ();
 
   /* Run any lang-specific selftests.  */
   lang_hooks.run_lang_selftests ();
diff --git a/gcc/selftest.h b/gcc/selftest.h
index dad53e9fe09..0572fefd281 100644
--- a/gcc/selftest.h
+++ b/gcc/selftest.h
@@ -196,6 +196,7 @@ extern void tree_c_tests ();
 extern void tree_cfg_c_tests ();
 extern void vec_c_tests ();
 extern void wide_int_cc_tests ();
+extern void predict_c_tests ();
 
 extern int num_passes;
 
-- 
2.13.0


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]