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][RFC] Param to options conversion (demo).

On 11/5/19 5:01 PM, Richard Biener wrote:

On Tue, Nov 5, 2019 at 4:22 PM Martin Liška <mliska@suse.cz> wrote:


On 11/5/19 3:13 PM, Richard Biener wrote:

On Thu, Oct 31, 2019 at 2:17 PM Martin Liška <mliska@suse.cz> wrote:


On 10/31/19 2:16 PM, Martin Liška wrote:

On 10/31/19 2:01 PM, Martin Liška wrote:

Hi.

Based on the discussion with Honza and Richard I'm sending a proposal
for conversion of param machinery into the existing option machinery.
Our motivation for the change is to provide per function param values,
similarly what 'Optimization' keyword does for options.

Right now, we support the following format:
gcc --param=lto-partitions=4 /tmp/main.c -c

And so that I decided to name newly the params like:

-param=ipa-sra-ptr-growth-factor=
Common Joined UInteger Var(param_ipa_sra_ptr_growth_factor) Init(2) Param Optimization
Maximum allowed growth of number and total size of new parameters
that ipa-sra replaces a pointer to an aggregate with.

And I learnt decoder to parse '--param' 'name=value' as '--param=name=value'. Doing that
the transformation works. Help provides reasonable output as well:

$ ./xgcc -B. --param predictable-branch-outcome=5  /tmp/main.c -c -Q --help=param
The --param option recognizes the following as parameters:
    --param=ipa-sra-ptr-growth-factor=         2
    --param=predictable-branch-outcome=<0,50>  5

Thoughts?
Thanks,
Martin

---
   gcc/common.opt        | 18 +++++++++++-------
   gcc/ipa-sra.c         |  3 +--
   gcc/opt-functions.awk |  3 ++-
   gcc/opts-common.c     |  9 +++++++++
   gcc/opts.c            | 36 ------------------------------------
   gcc/params.def        | 10 ----------
   gcc/predict.c         |  4 ++--
   7 files changed, 25 insertions(+), 58 deletions(-)




I forgot to add gcc-patches to To.

Martin



+ the patch.


Nice.


Thanks.



I wonder if we can auto-generate params.h so that
PARAM_VALUE (...) can continue to "work"?  But maybe that's too much
and against making them first-class (but "unsupported") options.  At least
it would make the final patch _much_ smaller... (one could think of
auto-generating an enum and using an array of params for the storage
again - but then possibly split for [non-]Optimization - ugh).  If we
(auto-)name
the variables all-uppercase like PARAM_IPA_SRA_PTR_GROWTH_FACTOR
we could have

#define PARAM_VALUE (x) x

... (that said, everything that helps making the transition hit GCC 10
is appreciated ;))


Well, to be honest I would like to get rid of the current syntax:
PARAM_VALUE (PARAM_PREDICTABLE_BRANCH_OUTCOME) and replace it with something
what we have for normal options: param_predictable_branch_outcome.
It will require a quite big mechanical change in usage, but I can do
the replacement almost automatically.


OK, the more interesting uses are probably maybe_set_param_value ...


Which is actually for free ;) Please see the attached patch where I introduced
a new macro SET_OPTION_IF_UNSET.

Do you like it so that I can transform current options with that:

-  if (!opts_set->x_flag_branch_probabilities)
-    opts->x_flag_branch_probabilities = value;
+  SET_OPTION_IF_UNSET (opts, opts_set, flag_branch_probabilities, value);

?





For

+-param=ipa-sra-ptr-growth-factor=
+Common Joined UInteger Var(param_ipa_sra_ptr_growth_factor) Init(2)
Param Optimization

I wonder if both Var(...) and Param can be "autodetected" (aka
actually required)?


Right now, Var is probably not required. Param can be handled by putting all
the params into a new param.opt file.


Param could be also autodetected from the name of the option (so could Var).


Well, looking at the current options, we are also quite explicit about various option flags.
I'll auto-generate the new params.opt file, so setting Var and Param will be for free.


Why is Var not required?


You are right, it's required.





At least the core of the patch looks nicely small!  How do the OPT_ enum values
for a --param look like?


Yep, I also like how small it is.

    OPT__param_ipa_sra_ptr_growth_factor_ = 62,/* --param=ipa-sra-ptr-growth-factor= */
    OPT__param_predictable_branch_outcome_ = 63,/* --param=predictable-branch-outcome= */


OK, looks fine.

So I guess go ahead unless somebody objects over the next weekend?  (in case not


Sure. I asked Honza about feedback and he looks happy with what I suggested.


you have another week to do the mechanical change?)  Maybe post a final patch
earlier w/o the mechanical work.


Martin



Richard.


Martin



Thanks,
Richard.


Martin





>From 0f56be4223a578268a8b7678393a3bc9609c3c1f Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Thu, 31 Oct 2019 13:53:54 +0100
Subject: [PATCH] Param to options conversion (demo).

---
 gcc/common.opt                 | 22 +++++++++++++------
 gcc/config/i386/i386-options.c |  6 ++----
 gcc/ipa-sra.c                  |  3 +--
 gcc/opt-functions.awk          |  3 ++-
 gcc/opts-common.c              |  9 ++++++++
 gcc/opts.c                     | 39 +---------------------------------
 gcc/opts.h                     |  6 ++++++
 gcc/params.def                 | 17 ---------------
 gcc/params.h                   |  2 --
 gcc/predict.c                  |  4 ++--
 gcc/tree-ssa-loop-prefetch.c   |  5 +++--
 11 files changed, 41 insertions(+), 75 deletions(-)

diff --git a/gcc/common.opt b/gcc/common.opt
index fdd923e3c35..01caa6a686f 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -437,13 +437,6 @@ Common Driver Alias(-target-help)
 fversion
 Common Driver Alias(-version)
 
--param
-Common Separate
---param <param>=<value>	Set parameter <param> to value.  See below for a complete list of parameters.
-
--param=
-Common Joined Alias(-param)
-
 -sysroot
 Driver Separate Alias(-sysroot=)
 
@@ -3346,4 +3339,19 @@ fipa-ra
 Common Report Var(flag_ipa_ra) Optimization
 Use caller save register across calls if possible.
 
+; PARAMS start here
+
+-param=predictable-branch-outcome=
+Common Joined UInteger Var(param_predictable_branch_outcome) Init(2) IntegerRange(0, 50) Param
+Maximal estimated outcome of branch considered predictable.
+
+-param=ipa-sra-ptr-growth-factor=
+Common Joined UInteger Var(param_ipa_sra_ptr_growth_factor) Init(2) Param Optimization
+Maximum allowed growth of number and total size of new parameters
+that ipa-sra replaces a pointer to an aggregate with.
+
+-param=l1-cache-size=
+Common Joined UInteger Var(param_l1_cache_size) Init(32) Param
+The size of L1 cache.
+
 ; This comment is to ensure we retain the blank line above.
diff --git a/gcc/config/i386/i386-options.c b/gcc/config/i386/i386-options.c
index dfc8ae23ba0..4cea84a28ea 100644
--- a/gcc/config/i386/i386-options.c
+++ b/gcc/config/i386/i386-options.c
@@ -2626,10 +2626,8 @@ ix86_option_override_internal (bool main_args_p,
 			 ix86_tune_cost->prefetch_block,
 			 opts->x_param_values,
 			 opts_set->x_param_values);
-  maybe_set_param_value (PARAM_L1_CACHE_SIZE,
-			 ix86_tune_cost->l1_cache_size,
-			 opts->x_param_values,
-			 opts_set->x_param_values);
+  SET_OPTION_IF_UNSET (opts, opts_set, param_l1_cache_size,
+		       ix86_tune_cost->l1_cache_size);
   maybe_set_param_value (PARAM_L2_CACHE_SIZE,
 			 ix86_tune_cost->l2_cache_size,
 			 opts->x_param_values,
diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c
index aceb5c722ea..3a27f4b798c 100644
--- a/gcc/ipa-sra.c
+++ b/gcc/ipa-sra.c
@@ -2280,8 +2280,7 @@ process_scan_results (cgraph_node *node, struct function *fun,
       if (!desc->by_ref || optimize_function_for_size_p (fun))
 	param_size_limit = cur_param_size;
       else
-	param_size_limit = (PARAM_VALUE (PARAM_IPA_SRA_PTR_GROWTH_FACTOR)
-			   * cur_param_size);
+	param_size_limit = param_ipa_sra_ptr_growth_factor * cur_param_size;
       if (nonarg_acc_size > param_size_limit
 	  || (!desc->by_ref && nonarg_acc_size == param_size_limit))
 	{
diff --git a/gcc/opt-functions.awk b/gcc/opt-functions.awk
index c1da80c648c..4f02b74e97c 100644
--- a/gcc/opt-functions.awk
+++ b/gcc/opt-functions.awk
@@ -105,7 +105,8 @@ function switch_flags (flags)
 	  test_flag("Undocumented", flags,  " | CL_UNDOCUMENTED") \
 	  test_flag("NoDWARFRecord", flags,  " | CL_NO_DWARF_RECORD") \
 	  test_flag("Warning", flags,  " | CL_WARNING") \
-	  test_flag("(Optimization|PerFunction)", flags,  " | CL_OPTIMIZATION")
+	  test_flag("(Optimization|PerFunction)", flags,  " | CL_OPTIMIZATION") \
+	  test_flag("Param", flags,  " | CL_PARAMS")
 	sub( "^0 \\| ", "", result )
 	return result
 }
diff --git a/gcc/opts-common.c b/gcc/opts-common.c
index b4ec1bd25ac..d55dc93e165 100644
--- a/gcc/opts-common.c
+++ b/gcc/opts-common.c
@@ -961,6 +961,15 @@ decode_cmdline_options_to_array (unsigned int argc, const char **argv,
 	  continue;
 	}
 
+      /* Interpret "--param" "key=name" as "--param=key=name".  */
+      const char *needle = "--param";
+      if (i + 1 < argc && strcmp (opt, needle) == 0)
+	{
+	  const char *replacement
+	    = opts_concat (needle, "=", argv[i + 1], NULL);
+	  argv[++i] = replacement;
+	}
+
       n = decode_cmdline_option (argv + i, lang_mask,
 				 &opt_array[num_decoded_options]);
       num_decoded_options++;
diff --git a/gcc/opts.c b/gcc/opts.c
index 10b9f108f8d..13e8d2709fe 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -1278,38 +1278,6 @@ print_filtered_help (unsigned int include_flags,
   bool displayed = false;
   char new_help[256];
 
-  if (include_flags == CL_PARAMS)
-    {
-      for (i = 0; i < LAST_PARAM; i++)
-	{
-	  const char *param = compiler_params[i].option;
-
-	  help = compiler_params[i].help;
-	  if (help == NULL || *help == '\0')
-	    {
-	      if (exclude_flags & CL_UNDOCUMENTED)
-		continue;
-	      help = undocumented_msg;
-	    }
-
-	  /* Get the translation.  */
-	  help = _(help);
-
-	  if (!opts->x_quiet_flag)
-	    {
-	      snprintf (new_help, sizeof (new_help),
-			_("default %d minimum %d maximum %d"),
-			compiler_params[i].default_value,
-			compiler_params[i].min_value,
-			compiler_params[i].max_value);
-	      help = new_help;
-	    }
-	  wrap_help (help, param, strlen (param), columns);
-	}
-      putchar ('\n');
-      return;
-    }
-
   if (!opts->x_help_printed)
     opts->x_help_printed = XCNEWVAR (char, cl_options_count);
 
@@ -1732,8 +1700,7 @@ enable_fdo_optimizations (struct gcc_options *opts,
 			  struct gcc_options *opts_set,
 			  int value)
 {
-  if (!opts_set->x_flag_branch_probabilities)
-    opts->x_flag_branch_probabilities = value;
+  SET_OPTION_IF_UNSET (opts, opts_set, flag_branch_probabilities, value);
   if (!opts_set->x_flag_profile_values)
     opts->x_flag_profile_values = value;
   if (!opts_set->x_flag_unroll_loops)
@@ -2241,10 +2208,6 @@ common_handle_option (struct gcc_options *opts,
 
   switch (code)
     {
-    case OPT__param:
-      handle_param (opts, opts_set, loc, arg);
-      break;
-
     case OPT__help:
       {
 	unsigned int all_langs_mask = (1U << cl_lang_count) - 1;
diff --git a/gcc/opts.h b/gcc/opts.h
index 47223229388..e3e7e78c206 100644
--- a/gcc/opts.h
+++ b/gcc/opts.h
@@ -461,4 +461,10 @@ extern bool parse_and_check_align_values (const char *flag,
 					  bool report_error,
 					  location_t loc);
 
+/* Set OPTION in OPTS to VALUE if the option is not set in OPTS_SET.  */
+
+#define SET_OPTION_IF_UNSET(OPTS, OPTS_SET, OPTION, VALUE) \
+  if (!OPTS_SET->x_ ## OPTION) \
+    OPTS->x_ ## OPTION = VALUE;
+
 #endif
diff --git a/gcc/params.def b/gcc/params.def
index 942447d77e6..370115534ce 100644
--- a/gcc/params.def
+++ b/gcc/params.def
@@ -44,10 +44,6 @@ along with GCC; see the file COPYING3.  If not see
 
 /* When branch is predicted to be taken with probability lower than this
    threshold (in percent), then it is considered well predictable. */
-DEFPARAM (PARAM_PREDICTABLE_BRANCH_OUTCOME,
-	  "predictable-branch-outcome",
-	  "Maximal estimated outcome of branch considered predictable.",
-	  2, 0, 50)
 
 DEFPARAM (PARAM_INLINE_MIN_SPEEDUP,
 	  "inline-min-speedup",
@@ -852,13 +848,6 @@ DEFPARAM (PARAM_SIMULTANEOUS_PREFETCHES,
 	  "The number of prefetches that can run at the same time.",
 	  3, 0, 0)
 
-/* The size of L1 cache in kB.  */
-
-DEFPARAM (PARAM_L1_CACHE_SIZE,
-	  "l1-cache-size",
-	  "The size of L1 cache.",
-	  64, 0, 0)
-
 /* The size of L1 cache line in bytes.  */
 
 DEFPARAM (PARAM_L1_CACHE_LINE_SIZE,
@@ -1085,12 +1074,6 @@ DEFPARAM (PARAM_MIN_NONDEBUG_INSN_UID,
 	  "The minimum UID to be used for a nondebug insn.",
 	  0, 0, 0)
 
-DEFPARAM (PARAM_IPA_SRA_PTR_GROWTH_FACTOR,
-	  "ipa-sra-ptr-growth-factor",
-	  "Maximum allowed growth of number and total size of new parameters "
-	  "that ipa-sra replaces a pointer to an aggregate with.",
-	  2, 0, 0)
-
 DEFPARAM (PARAM_IPA_SRA_MAX_REPLACEMENTS,
 	  "ipa-sra-max-replacements",
 	  "Maximum pieces that IPA-SRA tracks per formal parameter, as "
diff --git a/gcc/params.h b/gcc/params.h
index 1aaef6d6a00..ac0ba7ee2e3 100644
--- a/gcc/params.h
+++ b/gcc/params.h
@@ -190,8 +190,6 @@ extern void init_param_values (int *params);
   PARAM_VALUE (PARAM_PREFETCH_LATENCY)
 #define SIMULTANEOUS_PREFETCHES \
   PARAM_VALUE (PARAM_SIMULTANEOUS_PREFETCHES)
-#define L1_CACHE_SIZE \
-  PARAM_VALUE (PARAM_L1_CACHE_SIZE)
 #define L1_CACHE_LINE_SIZE \
   PARAM_VALUE (PARAM_L1_CACHE_LINE_SIZE)
 #define L2_CACHE_SIZE \
diff --git a/gcc/predict.c b/gcc/predict.c
index 915f0806b11..edb6920cfd5 100644
--- a/gcc/predict.c
+++ b/gcc/predict.c
@@ -412,9 +412,9 @@ predictable_edge_p (edge e)
   if (!e->probability.initialized_p ())
     return false;
   if ((e->probability.to_reg_br_prob_base ()
-       <= PARAM_VALUE (PARAM_PREDICTABLE_BRANCH_OUTCOME) * REG_BR_PROB_BASE / 100)
+       <= param_predictable_branch_outcome * REG_BR_PROB_BASE / 100)
       || (REG_BR_PROB_BASE - e->probability.to_reg_br_prob_base ()
-          <= PARAM_VALUE (PARAM_PREDICTABLE_BRANCH_OUTCOME) * REG_BR_PROB_BASE / 100))
+	  <= param_predictable_branch_outcome * REG_BR_PROB_BASE / 100))
     return true;
   return false;
 }
diff --git a/gcc/tree-ssa-loop-prefetch.c b/gcc/tree-ssa-loop-prefetch.c
index 04ff5244b69..a3a00e400ac 100644
--- a/gcc/tree-ssa-loop-prefetch.c
+++ b/gcc/tree-ssa-loop-prefetch.c
@@ -191,7 +191,7 @@ along with GCC; see the file COPYING3.  If not see
 #define ACCEPTABLE_MISS_RATE 50
 #endif
 
-#define L1_CACHE_SIZE_BYTES ((unsigned) (L1_CACHE_SIZE * 1024))
+#define L1_CACHE_SIZE_BYTES ((unsigned) (param_l1_cache_size * 1024))
 #define L2_CACHE_SIZE_BYTES ((unsigned) (L2_CACHE_SIZE * 1024))
 
 /* We consider a memory access nontemporal if it is not reused sooner than
@@ -2002,7 +2002,8 @@ tree_ssa_prefetch_arrays (void)
       fprintf (dump_file, "    prefetch latency: %d\n", PREFETCH_LATENCY);
       fprintf (dump_file, "    prefetch block size: %d\n", PREFETCH_BLOCK);
       fprintf (dump_file, "    L1 cache size: %d lines, %d kB\n",
-	       L1_CACHE_SIZE_BYTES / L1_CACHE_LINE_SIZE, L1_CACHE_SIZE);
+	       L1_CACHE_SIZE_BYTES / L1_CACHE_LINE_SIZE,
+	       param_l1_cache_size);
       fprintf (dump_file, "    L1 cache line size: %d\n", L1_CACHE_LINE_SIZE);
       fprintf (dump_file, "    L2 cache size: %d kB\n", L2_CACHE_SIZE);
       fprintf (dump_file, "    min insn-to-prefetch ratio: %d \n",
-- 
2.23.0
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]