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

Reply via email to