On Wed, Aug 31, 2022 at 12:56:25PM +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_values): 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.
2 spaces after . instead of just one
> (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.
> (initialize_icvs): New function to initialize a gomp_initial_icvs
> struct.
> (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_device_num): Definition of device numbers for _ALL, _DEV, and
> no suffix.
> (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):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.
> (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_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.
> * 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 (gomp_get_offload_icv_item): Get a list item of
> gomp_offload_icv_list.
> (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. */
> +const struct gomp_default_icv gomp_default_icv_values = {
> .nthreads_var = 1,
> .thread_limit_var = UINT_MAX,
> .run_sched_var = GFS_DYNAMIC,
> .run_sched_chunk_size = 1,
> .default_device_var = 0,
> - .dyn_var = false,
> .max_active_levels_var = 1,
> .bind_var = omp_proc_bind_false,
> + .nteams_var = 0,
> + .teams_thread_limit_var = 0,
> + .dyn_var = false
> +};
> +
> +struct gomp_task_icv gomp_global_icv = {
> + .nthreads_var = gomp_default_icv_values.nthreads_var,
> + .thread_limit_var = gomp_default_icv_values.thread_limit_var,
> + .run_sched_var = gomp_default_icv_values.run_sched_var,
> + .run_sched_chunk_size = gomp_default_icv_values.run_sched_chunk_size,
> + .default_device_var = gomp_default_icv_values.default_device_var,
> + .dyn_var = gomp_default_icv_values.dyn_var,
> + .max_active_levels_var = gomp_default_icv_values.max_active_levels_var,
> + .bind_var = gomp_default_icv_values.bind_var,
> .target_data = NULL
> };
I'm afraid this isn't pedantically valid C (it is valid in C++), but as GCC
supports it as an extension, let's go with it, libgomp is only compiled by GCC.
> /* As parse_unsigned_long_1, but always use getenv. */
>
> static bool
> -parse_unsigned_long (const char *name, unsigned long *pvalue, bool
> allow_zero)
> +parse_unsigned_long (const char *env, const char *val, void * const params[])
No space after * above.
> -/* As parse_int_1, but use getenv. */
> -
> static bool
> -parse_int (const char *name, int *pvalue, bool allow_zero)
> +parse_int (const char *env, const char *val, void * const params[])
Ditto (several times more in the patch).
> - char *env, *end;
> + unsigned long *p1stvalue = (unsigned long *) params[0];
> + unsigned long **pvalues = (unsigned long **) params[1];
> + unsigned long *pnvalues = (unsigned long*) params[2];
Space before *
> +/* Helper function for initialize_env. Extracts the device number from
> + an environment variable name. ENV is the complete environment variable.
> + DEV_NUM_PTR points to the start of the device number in the environment
> + variable string. DEV_NUM_LEN is the returned length of the device num
> + string. */
> +
> +static bool
> +get_device_num (char *env, char *dev_num_ptr, int *dev_num, int *dev_num_len)
> +{
> + char *end;
> + unsigned long val = strtoul (dev_num_ptr, &end, 10);
> + if (val > INT_MAX
> + || *end != '='
> + || (dev_num_ptr[0] == '0' && val != 0)
For the val == 0 case I think you also need to verify that
end == dev_num_ptr + 1, to avoid accepting
OMP_NUM_THREADS_DEV_00000=1
as
OMP_NUM_THREADS_DEV_0=1
The others have it through dev_num_ptr[0] > '0'...
So
|| (dev_num_ptr[0] == '0' && (val != 0 || end != dev_num_ptr + 1))
instead of the above line?
Or just
|| (dev_num_ptr[0] == '0' && end != dev_num_ptr + 1)
? Because if end == dev_num_ptr + 1, then val has to be 0...
> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.c-c++-common/omp-display-env-1.c
> @@ -0,0 +1,119 @@
> +
> +int
> +main (int argc, char *const *argv)
This still has the unnecessary "int argc, char *const *argv"
> +{
> + omp_display_env (1);
> + return 0;
> +}
> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.c-c++-common/omp-display-env-2.c
> @@ -0,0 +1,22 @@
> +/* { dg-do run } */
> +/* { dg-set-target-env-var OMP_NUM_TEAMS "42" } */
> +
> +/* This test checks if omp_display_env outputs the initial ICV values
> although
> + the value was updated. */
> +
> +#include <omp.h>
> +#include <stdlib.h>
> +
> +int
> +main (int argc, char *const *argv)
> +{
> + omp_display_env (1);
> + omp_set_num_teams (24);
> + if (omp_get_max_teams () != 24)
> + abort ();
> + omp_display_env (1);
And this one too.
> +
> + return 0;
> +}
> +
> +/* { dg-output ".*\\\[host] OMP_NUM_TEAMS = '42'.*\\\[host] OMP_NUM_TEAMS =
> '42'" } */
At least until dg-set-target-env-var is changed so that it supports
non-native setting of env vars too, I'm afraid this needs to be
guarded, so
/* { dg-output ".*\\\[host] OMP_NUM_TEAMS = '42'.*\\\[host] OMP_NUM_TEAMS =
'42'" { target native } } */
or so (didn't check if other tests need something like that too).
Otherwise LGTM.
Jakub
