[PATCH] OpenMP, libgomp: Environment variable syntax extension.

[Jakub Jelinek]

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