diff mbox

[PR67476] Add param parloops-schedule

Message ID 55F2D6E1.7030504@mentor.com
State New
Headers show

Commit Message

Tom de Vries Sept. 11, 2015, 1:28 p.m. UTC 
On 11/09/15 12:57, Jakub Jelinek wrote:
> On Fri, Sep 11, 2015 at 12:55:00PM +0200, Tom de Vries wrote:
>> >Hi,
>> >
>> >this patch adds a param parloops-schedule=<0-4>, which sets the omp schedule
>> >for loops paralellized by parloops.
>> >
>> >The <0-4> maps onto <static|dynamic|guided|auto|runtime>.
>> >
>> >Bootstrapped and reg-tested on x86_64.
>> >
>> >OK for trunk?
> I don't really like it, the mapping of the integers to the enum values
> is non-obvious and hard to remember.
> Perhaps add support for enumeration params if you want this instead?
>

This patch adds handling of a DEFPARAMENUM macro, which is similar to 
the DEFPARAM macro, but allows the values to be named.

So the definition of param parloop-schedule becomes:
...
DEFPARAMENUM PARAM_PARLOOPS_SCHEDULE,
              "parloops-schedule",
              "Schedule type of omp schedule for loops parallelized by "
              "parloops (static, dynamic, guided, auto, runtime)",
              0, 0, 4, "static", "dynamic", "guided", "auto", "runtime")
...
[ I'll repost the original patch containing this update. ]

OK for trunk if x86_64 bootstrap and reg-test succeeds?

Thanks,
- Tom

Comments

Bernd Schmidt Sept. 14, 2015, 8:52 a.m. UTC | #1 
On 09/11/2015 03:28 PM, Tom de Vries wrote:

> This patch adds handling of a DEFPARAMENUM macro, which is similar to
> the DEFPARAM macro, but allows the values to be named.
>
> So the definition of param parloop-schedule becomes:
> ...
> DEFPARAMENUM PARAM_PARLOOPS_SCHEDULE,
>               "parloops-schedule",
>               "Schedule type of omp schedule for loops parallelized by "
>               "parloops (static, dynamic, guided, auto, runtime)",
>               0, 0, 4, "static", "dynamic", "guided", "auto", "runtime")

So in principle I like this, but there's one oddity:

+  switch (schedule_type)
+    {
+    case 0:
+      OMP_CLAUSE_SCHEDULE_KIND (t) = OMP_CLAUSE_SCHEDULE_STATIC;
+      break;

The code using the param is using integers rather than enum values. Can 
that be fixed?

> ...
> [ I'll repost the original patch containing this update. ]

I'll let Jakub and/or Richard handle the rest of that. I'm curious why 
this would be a param rather than a -f option.


Bernd
Jakub Jelinek Sept. 14, 2015, 9:09 a.m. UTC | #2 
On Fri, Sep 11, 2015 at 03:28:01PM +0200, Tom de Vries wrote:
> On 11/09/15 12:57, Jakub Jelinek wrote:
> >On Fri, Sep 11, 2015 at 12:55:00PM +0200, Tom de Vries wrote:
> >>>Hi,
> >>>
> >>>this patch adds a param parloops-schedule=<0-4>, which sets the omp schedule
> >>>for loops paralellized by parloops.
> >>>
> >>>The <0-4> maps onto <static|dynamic|guided|auto|runtime>.
> >>>
> >>>Bootstrapped and reg-tested on x86_64.
> >>>
> >>>OK for trunk?
> >I don't really like it, the mapping of the integers to the enum values
> >is non-obvious and hard to remember.
> >Perhaps add support for enumeration params if you want this instead?
> >
> 
> This patch adds handling of a DEFPARAMENUM macro, which is similar to the
> DEFPARAM macro, but allows the values to be named.
> 
> So the definition of param parloop-schedule becomes:
> ...
> DEFPARAMENUM PARAM_PARLOOPS_SCHEDULE,
>              "parloops-schedule",
>              "Schedule type of omp schedule for loops parallelized by "
>              "parloops (static, dynamic, guided, auto, runtime)",
>              0, 0, 4, "static", "dynamic", "guided", "auto", "runtime")
> ...
> [ I'll repost the original patch containing this update. ]
> 
> OK for trunk if x86_64 bootstrap and reg-test succeeds?

That still allows numeric arguments for the param, which is IMHO
undesirable.  If it is enum kind, only the enum values should be accepted.
Also, it would be nice if params.h in that case would define an enum with
the values like
PARAM_PARLOOPS_SCHEDULE_KIND_{static,dynamic,guided,auto,runtime}, so use
values not wrapped in ""s and only in a macro or generator make both
enums and string array out of that.

There is also the question if we can use __VA_ARGS__, isn't that C99 or
C++11 and later feature?  I see gengtype.h and ipa-icf-gimple.h use
that too, so maybe yes, but am not sure.

	Jakub
Tom de Vries Sept. 14, 2015, 4:50 p.m. UTC | #3 
On 14/09/15 10:52, Bernd Schmidt wrote:
> I'm curious why
> this would be a param rather than a -f option.

Hi Bernd,

parloops-chunk-size is also a param, so I think it would make sense to 
have parloops-schedule as a param as well.
[ So, in order for parloops to generate:
     #pragma omp for schedule(dynamic,100)
   we specify on the command line:
     --param parloops-schedule=dynamic --param parloops-chunk-size=100 ]

But in general, I don't really know how I should choose between:
* --param parm=<parmvalues>, and
* -fparm=<parmvalues>.

Thanks,
- Tom
diff mbox

Patch

Support DEFPARAMENUM in params.def

2015-09-11  Tom de Vries  <tom@codesourcery.com>

	* opts.c (handle_param): Handle case that param arg is a string.
	* params-list.h: Handle DEFPARAMENUM in params.def.
	* params.c (find_param): New function, factored out of ...
	(set_param_value): ... here.
	(get_param_string_value): New function.
	* params.h (struct param_info): Add values field.
	(get_param_string_value): Declare.
---
 gcc/opts.c        | 12 ++++---
 gcc/params-list.h |  3 ++
 gcc/params.c      | 93 +++++++++++++++++++++++++++++++++++++++++--------------
 gcc/params.h      |  5 +++
 4 files changed, 85 insertions(+), 28 deletions(-)

diff --git a/gcc/opts.c b/gcc/opts.c
index f1a9acd..47b8b86 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -2116,15 +2116,17 @@  handle_param (struct gcc_options *opts, struct gcc_options *opts_set,
 	      arg);
   else
     {
+      *equal = '\0';
+
       value = integral_argument (equal + 1);
       if (value == -1)
+	value = get_param_string_value (arg, equal + 1);
+
+      if (value == -1)
 	error_at (loc, "invalid --param value %qs", equal + 1);
       else
-	{
-	  *equal = '\0';
-	  set_param_value (arg, value,
-			   opts->x_param_values, opts_set->x_param_values);
-	}
+	set_param_value (arg, value,
+			 opts->x_param_values, opts_set->x_param_values);
     }
 
   free (arg);
diff --git a/gcc/params-list.h b/gcc/params-list.h
index ee33ef5..8a19919 100644
--- a/gcc/params-list.h
+++ b/gcc/params-list.h
@@ -19,5 +19,8 @@  along with GCC; see the file COPYING3.  If not see
 
 #define DEFPARAM(enumerator, option, nocmsgid, default, min, max) \
   enumerator,
+#define DEFPARAMENUM(enumerator, option, nocmsgid, default, min, max, ...) \
+  enumerator,
 #include "params.def"
 #undef DEFPARAM
+#undef DEFPARAMENUM
diff --git a/gcc/params.c b/gcc/params.c
index b0bc80b..7eedab8 100644
--- a/gcc/params.c
+++ b/gcc/params.c
@@ -37,12 +37,22 @@  static size_t num_compiler_params;
    default values determined.  */
 static bool params_finished;
 
+#define DEFPARAM(ENUM, OPTION, HELP, DEFAULT, MIN, MAX)
+#define DEFPARAMENUM(ENUM, OPTION, HELP, DEFAULT, MIN, MAX, ...)	\
+  static const char *values_ ## ENUM [] = { __VA_ARGS__ };
+#include "params.def"
+#undef DEFPARAMENUM
+#undef DEFPARAM
+
 static const param_info lang_independent_params[] = {
 #define DEFPARAM(ENUM, OPTION, HELP, DEFAULT, MIN, MAX) \
-  { OPTION, DEFAULT, MIN, MAX, HELP },
+  { OPTION, DEFAULT, MIN, MAX, HELP, NULL },
+#define DEFPARAMENUM(ENUM, OPTION, HELP, DEFAULT, MIN, MAX, ...)	\
+  { OPTION, DEFAULT, MIN, MAX, HELP, values_ ## ENUM },
 #include "params.def"
 #undef DEFPARAM
-  { NULL, 0, 0, 0, NULL }
+#undef DEFPARAMENUM
+  { NULL, 0, 0, 0, NULL, NULL }
 };
 
 /* Add the N PARAMS to the current list of compiler parameters.  */
@@ -114,6 +124,45 @@  set_param_value_internal (compiler_param num, int value,
     params_set[i] = true;
 }
 
+/* Return true if it can find the matching entry for NAME in the parameter
+   table, and assign the entry index to INDEX.  Return false otherwise.  */
+
+bool
+find_param (const char *name, size_t *index)
+{
+  for (size_t i = 0; i < num_compiler_params; ++i)
+    if (strcmp (compiler_params[i].option, name) == 0)
+      {
+	*index = i;
+	return true;
+      }
+
+  return false;
+}
+
+/* Return the param value for param name VALUE_NAME belonging to param NAME.
+   Return -1 if there no corresponding param value.  */
+
+int
+get_param_string_value (const char *name, const char *value_name)
+{
+  size_t index;
+  if (!find_param (name, &index))
+    return -1;
+
+  param_info *entry = &compiler_params[index];
+  if (entry->value_names == NULL)
+    return -1;
+
+  int n = entry->max_value - entry->min_value + 1;
+  int value = entry->min_value;
+  for (int i = 0; i < n ; ++i, ++value)
+    if (strcmp (entry->value_names[i], value_name) == 0)
+      return value;
+
+  return -1;
+}
+
 /* Set the VALUE associated with the parameter given by NAME in PARAMS
    and PARAMS_SET.  */
 
@@ -126,27 +175,25 @@  set_param_value (const char *name, int value,
   /* Make sure nobody tries to set a parameter to an invalid value.  */
   gcc_assert (value != INVALID_PARAM_VAL);
 
-  /* Scan the parameter table to find a matching entry.  */
-  for (i = 0; i < num_compiler_params; ++i)
-    if (strcmp (compiler_params[i].option, name) == 0)
-      {
-	if (value < compiler_params[i].min_value)
-	  error ("minimum value of parameter %qs is %u",
-		 compiler_params[i].option,
-		 compiler_params[i].min_value);
-	else if (compiler_params[i].max_value > compiler_params[i].min_value
-		 && value > compiler_params[i].max_value)
-	  error ("maximum value of parameter %qs is %u",
-		 compiler_params[i].option,
-		 compiler_params[i].max_value);
-	else
-	  set_param_value_internal ((compiler_param) i, value,
-				    params, params_set, true);
-	return;
-      }
-
-  /* If we didn't find this parameter, issue an error message.  */
-  error ("invalid parameter %qs", name);
+  if (!find_param (name, &i))
+    {
+      /* If we didn't find this parameter, issue an error message.  */
+      error ("invalid parameter %qs", name);
+      return;
+    }
+
+  if (value < compiler_params[i].min_value)
+    error ("minimum value of parameter %qs is %u",
+	   compiler_params[i].option,
+	   compiler_params[i].min_value);
+  else if (compiler_params[i].max_value > compiler_params[i].min_value
+	   && value > compiler_params[i].max_value)
+    error ("maximum value of parameter %qs is %u",
+	   compiler_params[i].option,
+	   compiler_params[i].max_value);
+  else
+    set_param_value_internal ((compiler_param) i, value,
+			      params, params_set, true);
 }
 
 /* Set the value of the parameter given by NUM to VALUE in PARAMS and
diff --git a/gcc/params.h b/gcc/params.h
index 9f7618a..cec6131 100644
--- a/gcc/params.h
+++ b/gcc/params.h
@@ -55,6 +55,9 @@  struct param_info
 
   /* A short description of the option.  */
   const char *const help;
+
+  /* The optional names corresponding to the values.  */
+  const char **value_names;
 };
 
 /* An array containing the compiler parameters and their current
@@ -69,6 +72,8 @@  extern size_t get_num_compiler_params (void);
 
 extern void add_params (const param_info params[], size_t n);
 
+extern int get_param_string_value (const char *, const char*);
+
 /* Set the VALUE associated with the parameter given by NAME in the
    table PARAMS using PARAMS_SET to indicate which have been
    explicitly set.  */
-- 
1.9.1