On Tue, Aug 02, 2022 at 09:52:02AM +0200, Marcel Vollweiler wrote:
> libgomp/ChangeLog:
>
> * config/gcn/icv-device.c (omp_get_default_device): Return device-
> specific ICV.
> (omp_get_max_teams): Added for GCN devices.
> (omp_set_num_teams): Likewise.
> (ialias): Likewise.
> * config/nvptx/icv-device.c (omp_get_default_device): Return device-
> specific ICV.
> (omp_get_max_teams): Added for NVPTX devices.
> (omp_set_num_teams): Likewise.
> (ialias): Likewise.
> * env.c (struct gomp_icv_list): New struct to store entries of initial
> ICV values.
> (struct gomp_offload_icv_list): New struct to store entries of device-
> specific ICV values that are copied to the device and back.
> (struct gomp_default_icv_t): New struct to store default values of ICVs
> according to the OpenMP standard.
> (parse_schedule): Generalized for different variants of OMP_SCHEDULE.
> (print_env_var_error): Function that prints an error for invalid values
> for ICVs.
> (parse_unsigned_long_1): Removed getenv. Generalized.
> (parse_unsigned_long): Likewise.
> (parse_int_1): Likewise.
> (parse_int): Likewise.
> (parse_int_secure): Likewise.
> (parse_unsigned_long_list): Likewise.
> (parse_target_offload): Likewise.
> (parse_bind_var): Likewise.
> (parse_stacksize): Likewise.
> (parse_boolean): Likewise.
> (parse_wait_policy): Likewise.
> (parse_allocator): Likewise.
> (omp_display_env): Extended to output different variants of environment
> variables.
> (print_schedule): New helper function for omp_display_env which prints
> the values of run_sched_var.
> (print_proc_bind): New helper function for omp_display_env which prints
> the values of proc_bind_var.
> (enum gomp_parse_type): Collection of types used for parsing environment
> variables.
> (ENTRY): Preprocess string lengths of environment variables.
> (OMP_VAR_CNT): Preprocess table size.
> (OMP_HOST_VAR_CNT): Likewise.
> (INT_MAX_STR_LEN): Constant for the maximal number of digits of a device
> number.
> (gomp_get_icv_flag): Returns if a flag for a particular ICV is set.
> (gomp_set_icv_flag): Sets a flag for a particular ICV.
> (print_device_specific_icvs): New helper function for omp_display_env to
> print device specific ICV values.
> (get_device_num): New helper function for parse_device_specific.
> Extracts the device number from an environment variable name.
> (get_icv_member_addr): Gets the memory address for a particular member
> of an ICV struct.
> (gomp_get_initial_icv_item): Get a list item of gomp_initial_icv_list.
> (gomp_get_offload_icv_item): Get a list item of gomp_offload_icv_list.
> (add_initial_icv_to_list): Adds an ICV struct to gomp_initial_icv_list.
> (startswith): Checks if a string starts with a given prefix.
> (initialize_env): Extended to parse the new syntax of environment
> variables.
> * icv-device.c (omp_get_max_teams): Added.
> (ialias): Likewise.
> (omp_set_num_teams): Likewise.
> * icv.c (omp_set_num_teams): Moved to icv-device.c.
> (omp_get_max_teams): Likewise.
> (ialias): Likewise.
> * libgomp-plugin.h (GOMP_DEVICE_NUM_VAR): Removed.
> (GOMP_ADDITIONAL_ICVS): New target-side struct that
> holds the designated ICVs of the target device.
> * libgomp.h (enum gomp_icvs): Collection of ICVs.
> (enum gomp_env_suffix): Collection of possible suffixes of environment
> variables.
> (struct gomp_initial_icvs): Contains all ICVs for which we need to store
> initial values.
> (struct gomp_default_icv_t): New struct to hold ICVs for which we need
> to store initial values.
> (struct gomp_icv_list): Definition of a linked list that is used for
> storing ICVs for the devices and also for _DEV, _ALL, and without
> suffix.
> (gomp_get_initial_icv_item): Get a list item of gomp_initial_icv_list.
> (gomp_get_icv_flag): Returns if a flag for a particular ICV is set.
> (struct gomp_offload_icvs): New struct to hold ICVs that are copied to
> a device.
> (struct gomp_offload_icv_list): Definition of a linked list that holds
> device-specific ICVs that are copied to devices.
> (gomp_get_offload_icv_item): Get a list item of gomp_offload_icv_list.
> * plugin/plugin-gcn.c (GOMP_OFFLOAD_load_image): Extended to read
> further ICVs from the offload image.
> * plugin/plugin-nvptx.c (GOMP_OFFLOAD_load_image): Likewise.
> * target.c (get_gomp_offload_icvs): New. Returns the ICV values
> depending on the device num and the variable hierarchy.
> (gomp_load_image_to_device): Extended to copy further ICVs to a device.
> * testsuite/libgomp.c-c++-common/icv-5.c: New test.
> * testsuite/libgomp.c-c++-common/icv-6.c: New test.
> * testsuite/libgomp.c-c++-common/icv-7.c: New test.
> * testsuite/libgomp.c-c++-common/icv-8.c: New test.
> * testsuite/libgomp.c-c++-common/omp-display-env-1.c: New test.
> * testsuite/libgomp.c-c++-common/omp-display-env-2.c: New test.
>
> +/* Default values of ICVs according to the OpenMP standard. */
> +struct gomp_default_icv_t gomp_default_icv_values = {
> + .run_sched_var = GFS_DYNAMIC,
> + .run_sched_chunk_size = 1,
> + .max_active_levels_var = 1,
> + .bind_var = omp_proc_bind_false,
> + .nteams_var = 0,
> + .teams_thread_limit_var = 0,
> + .default_device_var = 0
> +};
Why this var (and if it is really needed, why it isn't const)?
You seem to be using only 2 fields from it:
libgomp/libgomp.h:extern struct gomp_default_icv_t gomp_default_icv_values;
libgomp/env.c:struct gomp_default_icv_t gomp_default_icv_values = {
libgomp/target.c: new->icvs.nteams = gomp_default_icv_values.nteams_var;
libgomp/target.c: new->icvs.default_device =
gomp_default_icv_values.default_device_var;
> +
> bool gomp_cancel_var = false;
> enum gomp_target_offload_t gomp_target_offload_var
> = GOMP_TARGET_OFFLOAD_DEFAULT;
> @@ -104,86 +123,94 @@ int goacc_default_dims[GOMP_DIM_MAX];
> static int wait_policy;
> static unsigned long stacksize = GOMP_DEFAULT_STACKSIZE;
>
> -/* Parse the OMP_SCHEDULE environment variable. */
> -
> static void
> -parse_schedule (void)
> +print_env_var_error (const char *env, const char *val)
> {
> - char *env, *end;
> + char name[val - env];
> + memcpy (name, env, val - env - 1);
> + name[val - env - 1] = '\0';
> + gomp_error ("Invalid value for environment variable %s: %s", name, val);
Why the temporary buffer (especially VLA)?
Just
gomp_error ("Invalid value for environment variable %.*s: %s",
(int) (val - env - 1), env, val);
should do the job.
> +/* Parse the OMP_SCHEDULE environment variable. */
> +static bool
> +parse_schedule (const char *env, const char *val, void * const params[])
No space after *
> +#define ENTRY(NAME) NAME, sizeof (NAME) - 1
> +static const struct envvar
> +{
> + const char *name;
> + int name_len;
> + uint8_t flag_vars[3];
> + uint8_t flag;
> + bool (*parse_func) (const char *, const char *, void * const[]);
> +} envvars[] = {
> + { ENTRY ("OMP_SCHEDULE"),
> + { GOMP_ICV_SCHEDULE, GOMP_ICV_SCHEDULE_CHUNK_SIZE },
> + GOMP_ENV_SUFFIX_DEV | GOMP_ENV_SUFFIX_ALL | GOMP_ENV_SUFFIX_DEV_X,
> + &parse_schedule },
> + { ENTRY ("OMP_NUM_TEAMS"),
> + { GOMP_ICV_NTEAMS },
> + GOMP_ENV_SUFFIX_DEV | GOMP_ENV_SUFFIX_ALL | GOMP_ENV_SUFFIX_DEV_X,
> + &parse_int },
> + { ENTRY ("OMP_DYNAMIC"),
> + { GOMP_ICV_DYNAMIC },
> + GOMP_ENV_SUFFIX_DEV | GOMP_ENV_SUFFIX_ALL | GOMP_ENV_SUFFIX_DEV_X,
> + &parse_boolean },
> + { ENTRY ("OMP_TEAMS_THREAD_LIMIT"),
> + { GOMP_ICV_TEAMS_THREAD_LIMIT },
> + GOMP_ENV_SUFFIX_DEV | GOMP_ENV_SUFFIX_ALL | GOMP_ENV_SUFFIX_DEV_X,
> + &parse_int },
> + { ENTRY ("OMP_THREAD_LIMIT"),
> + { GOMP_ICV_THREAD_LIMIT },
> + GOMP_ENV_SUFFIX_DEV | GOMP_ENV_SUFFIX_ALL | GOMP_ENV_SUFFIX_DEV_X,
> + &parse_unsigned_long },
> + { ENTRY ("OMP_NUM_THREADS"),
> + { GOMP_ICV_NTHREADS, GOMP_ICV_NTHREADS_LIST, GOMP_ICV_NTHREADS_LIST_LEN
> },
> + GOMP_ENV_SUFFIX_DEV | GOMP_ENV_SUFFIX_ALL | GOMP_ENV_SUFFIX_DEV_X,
> + &parse_unsigned_long_list },
> + { ENTRY ("OMP_PROC_BIND"),
> + { GOMP_ICV_BIND, GOMP_ICV_BIND_LIST, GOMP_ICV_BIND_LIST_LEN },
> + GOMP_ENV_SUFFIX_DEV | GOMP_ENV_SUFFIX_ALL | GOMP_ENV_SUFFIX_DEV_X,
> + &parse_bind_var },
> + { ENTRY ("OMP_MAX_ACTIVE_LEVELS"),
> + { GOMP_ICV_MAX_ACTIVE_LEVELS },
> + GOMP_ENV_SUFFIX_DEV | GOMP_ENV_SUFFIX_ALL | GOMP_ENV_SUFFIX_DEV_X,
> + &parse_unsigned_long },
> + { ENTRY ("OMP_WAIT_POLICY"),
> + { GOMP_ICV_WAIT_POLICY },
> + GOMP_ENV_SUFFIX_DEV | GOMP_ENV_SUFFIX_ALL | GOMP_ENV_SUFFIX_DEV_X,
> + &parse_wait_policy },
> + { ENTRY ("OMP_STACKSIZE"),
> + { GOMP_ICV_STACKSIZE },
> + GOMP_ENV_SUFFIX_DEV | GOMP_ENV_SUFFIX_ALL | GOMP_ENV_SUFFIX_DEV_X,
> + &parse_stacksize },
> + { ENTRY ("OMP_CANCELLATION"), { GOMP_ICV_CANCELLATION }, 0, &parse_boolean
> },
> + { ENTRY ("OMP_DISPLAY_AFFINITY"), { GOMP_ICV_DISPLAY_AFFINITY }, 0,
> + &parse_boolean },
> + { ENTRY ("OMP_TARGET_OFFLOAD"), { GOMP_ICV_TARGET_OFFLOAD }, 0,
> + &parse_target_offload },
> + { ENTRY ("OMP_MAX_TASK_PRIORITY"), { GOMP_ICV_MAX_TASK_PRIORITY }, 0,
> + &parse_int },
> + { ENTRY ("OMP_ALLOCATOR"), { GOMP_ICV_ALLOCATOR }, 0, &parse_allocator },
> + { ENTRY ("OMP_DEFAULT_DEVICE"), { GOMP_ICV_DEFAULT_DEVICE }, 0, &parse_int
> }
All the entries in the table start with "OMP_" prefix, isn't it wasteful
to include that prefix in name field and name_len?
I mean, places that use the name to print it can just use OMP_%s and the
matching doesn't need to compare it again.
> +static bool
> +get_device_num (char *env, char *dev_num_ptr, int *dev_num, int *dev_num_len)
> +{
> + char *end;
> + int pos = 0;
> +
> + if (dev_num_ptr[0] == '-')
> + {
> + gomp_error ("Non-negative device number expected in %s", env);
> + return false;
> + }
> +
> + while (pos <= INT_MAX_STR_LEN)
> + {
> + if (dev_num_ptr[pos] == '\0' || dev_num_ptr[pos] == '=')
> + break;
> + pos++;
> + }
> + if (pos > INT_MAX_STR_LEN)
> + {
> + gomp_error ("Invalid device number in %s (too long)", env);
> + return false;
> + }
I don't understand the purpose of the above loop and if.
> +
> + *dev_num = (int) strtoul (dev_num_ptr, &end, 10);
Just call strtoul and verify afterwards using end you get, no need to walk
it separately.
I wouldn't cast to int before verification that it fits though.
So perhaps
char *end;
unsigned long val = strtoul (dev_num_ptr, &end, 10);
if (val > INT_MAX
|| *end != '='
|| (dev_num_ptr[0] == '0' && val != 0)
|| (dev_num_ptr[0] < '0' || dev_num_ptr[0] > '9'))
{
gomp_error ("Invalid device number in %s", env);
return false;
}
*dev_num = val;
*dev_num_len = end - dev_num_ptr;
return true;
or so? No need to differentiate different cases, and you want to
rule out "OMP_SCHEDULE_DEV_ \t+3=dynamic,3" too.
> + if (dev_num_ptr[0] == '0' && *dev_num != 0)
> + {
> + gomp_error ("Invalid device number in %s (leading zero)", env);
> + return false;
> + }
> + if (dev_num_ptr == end || *end != '=')
> + {
> + gomp_error ("Invalid device number in %s", env);
> + return false;
> + }
> +
> + *dev_num_len = pos;
> + return true;
> +}
> +
> +static uint32_t *
> +add_initial_icv_to_list (int dev_num, int icv_code, void *icv_addr[3])
> +{
> + struct gomp_icv_list *last = NULL, *l = gomp_initial_icv_list;
> + while (l != NULL && l->device_num != dev_num)
> + {
> + last = l;
> + l = l->next;
> + }
> +
> + if (l == NULL)
> + {
> + l
> + = (struct gomp_icv_list *) gomp_malloc (sizeof (struct gomp_icv_list));
What is the advantage of adding there a newline after the var name?
The = is indented 2 columns to the right from var name, so having it on one
line has the same length.
> + l->device_num = dev_num;
> + memset (&(l->icvs), 0, sizeof (struct gomp_initial_icvs));
You could use gomp_malloc_cleared (then format it as
l = ((struct gomp_icv_list *)
gomp_malloc_cleared (sizeof (struct gomp_icv_list)));
> + l->flags = 0;
> + if (dev_num < 0)
> + {
> + l->next = gomp_initial_icv_list;
> + gomp_initial_icv_list = l;
> + }
> + else
> + {
> + l->next = NULL;
> + if (last == NULL)
> + gomp_initial_icv_list = l;
> + else
> + last->next = l;
> + }
> + }
> +
> + get_icv_member_addr (&(l->icvs), icv_code, icv_addr);
No need for the ()s around l->icvs, &l->icvs will do (multiple places,
for other fields and other pointers too).
> +
> + return &(l->flags);
Similarly.
> +
> + /* Initial values for host environment variables should always exist even
> if
> + there is no explicitly set host environment variable. Moreover, they
> are
> + set to the initial global values. */
> + add_initial_icv_to_list (-3, 0, NULL);
> + none = gomp_get_initial_icv_item (-3);
Can you please add an enum for these -3/-2/-1 values and use it?
> + none->icvs.nthreads_var = 1;
> + none->icvs.thread_limit_var = UINT_MAX;
> + none->icvs.run_sched_var = GFS_DYNAMIC;
> + none->icvs.run_sched_chunk_size = 1;
> + none->icvs.default_device_var = 0;
> + none->icvs.dyn_var = false;
> + none->icvs.max_active_levels_var = 1;
> + none->icvs.bind_var = omp_proc_bind_false;
So, is this essentially a 3rd copy of the defaults?
gomp_global_icv has it, gomp_default_icv_values (partially) and
this spot too. Don't we want to have it initialized once and copy over?
> +
> + for (env = environ; *env != 0; env++)
> + {
> + if (!startswith (*env, "OMP_"))
> + continue;
> +
> + for (omp_var = 0; omp_var < OMP_VAR_CNT; omp_var++)
> + {
> + if (startswith (*env, envvars[omp_var].name))
As I said elsewhere,
if (startswith (*env + sizeof ("OMP_") - 1, envvars[omp_var].name))
instead?
Or have a temporary var set to *env + sizeof ("OMP_") - 1; and use
it instead of *env.
The amount of &(*env)[ etc. spots that could be just &name[ is big.
> + {
> + pos = envvars[omp_var].name_len;
> + if ((*env)[pos] == '=')
> + {
> + pos++;
> + flag_var_addr
> + = add_initial_icv_to_list (-3,
> + envvars[omp_var].flag_vars[0],
> + params);
> + }
> + else if (startswith (&(*env)[pos], "_DEV=")
> + && envvars[omp_var].flag & GOMP_ENV_SUFFIX_DEV)
> + {
> + pos += 5;
> + flag_var_addr
> + = add_initial_icv_to_list (-1,
> + envvars[omp_var].flag_vars[0],
> + params);
> + }
> + else if (startswith (&(*env)[pos], "_ALL=")
> + && envvars[omp_var].flag & GOMP_ENV_SUFFIX_ALL)
> + {
> + pos += 5;
> + flag_var_addr
> + = add_initial_icv_to_list (-2,
> + envvars[omp_var].flag_vars[0],
> + params);
> + }
> + else if (startswith (&(*env)[pos], "_DEV_")
> + && envvars[omp_var].flag & GOMP_ENV_SUFFIX_DEV_X)
> + {
> + pos += 5;
> + if (!get_device_num (*env, &(*env)[pos], &dev_num,
> + &dev_num_len))
> + goto next_var;
Isn't goto next_var; equivalent to just break; ?
> +
> + pos += dev_num_len + 1;
> + flag_var_addr
> + = add_initial_icv_to_list (dev_num,
> + envvars[omp_var].flag_vars[0],
> + params);
> + }
> + else
> + {
> + gomp_error ("Invalid device number in %s", *env);
> + break;
> + }
> + env_val = &(*env)[pos];
> +
> + if (envvars[omp_var].parse_func (*env, env_val, params))
> + {
> + for (i = 0; i < 3; ++i)
> + if (envvars[omp_var].flag_vars[i])
> + gomp_set_icv_flag (flag_var_addr,
> + envvars[omp_var].flag_vars[i]);
> + else
> + break;
> + }
> +
> + break;
> + }
> + }
> +
> + next_var:
So by using break; above we could drop this.
> +struct gomp_default_icv_t
> +{
> + enum gomp_schedule_type run_sched_var;
> + int run_sched_chunk_size;
> + unsigned char max_active_levels_var;
> + char bind_var;
> + int nteams_var;
> + int teams_thread_limit_var;
> + int default_device_var;
> +};
> +extern struct gomp_default_icv_t gomp_default_icv_values;
Please don't mix the type definitions with extern declarations and
with function prototypes.
Function prototypes should go to the section of libgomp.h that starts
with /* Function prototypes. */ comment and should go under section
corresponding to the filename that provides the definition.
For externs, there is a large block of extern declarations.
> + struct gomp_icv_list
> + {
> + int device_num;
> + struct gomp_initial_icvs icvs;
> + uint32_t flags;
Put flags after device_num to avoid some useless padding?
> + struct gomp_icv_list *next;
> + };
> +extern struct gomp_icv_list* gomp_add_device_specific_icv
* should go after space, not before.
> +int
> +main (int argc, char *const *argv)
No need for "int argc, char *const *argv" when you aren't using them,
just
int
main ()
> +{
> + omp_display_env (1);
> + omp_set_num_teams (24);
> + if (omp_get_max_teams () != 24)
> + abort ();
> + omp_display_env (1);
> +
> + return 0;
> +}
> +
> +/* { dg-output ".*\\\[host] OMP_NUM_TEAMS = '42'.*\\\[host] OMP_NUM_TEAMS =
> '42'" } */
Jakub
