Hi Jakub,

>>> I'm not sure we can rely on execv on all targets that do support libgomp.
>>> Any reason why you actually need this, rather than using
>>> dg-set-target-env-var directive(s) and perhaps return 0; if getenv doesn't
>>> return the expected values?
>>
>> Interesting topic. After some (internal) discussions I think the best way is to
>> set the environment variables explicitely instead using dg-set-target-env-var.
>> The reason is that dg-set-target-env-var does not work for remote testing (which
>> seems to be a common test environment). For remote testing dejagnu immediately
>> aborts the test case with UNSUPPORTED which is specified in the corresponding
>> extension and makes sence from my point of view as the test assumption cannot be
>> fulfilled (since the environment variables are not set on remote targets).
>> It also means that whenever dg-set-target-env-var is set in the test file, the
>> execution of the test case is not tested on remote targets.
>
> The only reason why dg-set-target-env-var is supported on native only right
> now is that I'm never doing remote testing myself and so couldn't test that.
> There is no inherent reason why the env vars couldn't be propagated over to
> the remote and set in the environment there.
> So trying to work around that rather than at least trying to change
> dg-set-target-env-var so that it works with the remote testing you do looks
> wrong.
> If dg-set-target-env-var can be made to work remotely, it will magically
> improve those 130+ tests that use it already together with the newly added
> tests.
>
> So, I'd suggest to just use dg-set-target-env-var and incrementally work on
> making it work for remote testing if that is important to whomever does
> that kind of testing.  Could be e.g. a matter of invoking remotely
> env VAR1=val1 VAR2=val2 program args
> instead of program args.  If env is missing on the remote side, it could
> be UNSUPPORTED then.

I agree. So I changed the tests using dg-set-target-env-var and removed the
execv parts.

>
>> +/* The initial ICV values for the host, which are configured with environment
>> +   variables without a suffix, e.g. OMP_NUM_TEAMS.  */
>> +struct gomp_initial_icvs gomp_initial_icvs_none;
>> +
>> +/* Initial ICV values that were configured for the host and for all devices by
>> +   using environment variables like OMP_NUM_TEAMS_ALL.  */
>> +struct gomp_initial_icvs gomp_initial_icvs_all;
>> +
>> +/* Initial ICV values that were configured only for devices (not for the host)
>> +   by using environment variables like OMP_NUM_TEAMS_DEV.  */
>> +struct gomp_initial_icvs gomp_initial_icvs_dev;
>
> As I said last time, I don't like allocating these
> all the time in the data section of libgomp when at least for a few upcoming
> years, most users will never use those suffixes.
> Can't *_DEV and *_ALL go into the gomp_initial_icv_dev_list
> chain too, perhaps

gomp_initial_icvs_{none, all, dev} are now defined as pointers (as you proposed
previously). gomp_initial_icvs_{all, dev} are only instantiated if at least one
according environment variable is parsed. gomp_initial_icvs_none is always
initialized with the initial global ICV values.

All three structures are now also included in gomp_initial_icv_list (previously
named gomp_initial_icv_dev_list) with "magic device numbers" -1, -2, and -3.
The list items for _DEV, _ALL and no suffix are stored at the beginning of the
list whereas the device-specific list items are attached at the end.

>
>> +static const struct envvar
>> +{
>> +  const char *name;
>> +  int name_len;
>> +  unsigned char flag_vars[3];
>> +  unsigned char flag;
>> +  void *params[3];
>> +  bool (*parse_func) (const char *, const char *, void * const[]);
>> +} envvars[] = {
>> +  {ENTRY ("OMP_SCHEDULE_DEV"), {OMP_SCHEDULE_DEV_, OMP_SCHEDULE_CHUNK_SIZE_DEV_}, GOMP_ENV_VAR_SUFFIX_DEV, {&gomp_initial_icvs_dev.run_sched_var, &gomp_initial_icvs_dev.run_sched_chunk_size}, &parse_schedule},
>> +  {ENTRY ("OMP_SCHEDULE_ALL"), {OMP_SCHEDULE_DEV_, OMP_SCHEDULE_CHUNK_SIZE_DEV_}, GOMP_ENV_VAR_SUFFIX_ALL, {&gomp_initial_icvs_all.run_sched_var, &gomp_initial_icvs_all.run_sched_chunk_size}, &parse_schedule},
>> +  {ENTRY ("OMP_SCHEDULE"), {OMP_SCHEDULE_DEV_, OMP_SCHEDULE_CHUNK_SIZE_DEV_}, GOMP_ENV_VAR_SUFFIX_NONE, {&gomp_initial_icvs_none.run_sched_var, &gomp_initial_icvs_none.run_sched_chunk_size}, &parse_schedule},
>> +
>> +  {ENTRY ("OMP_NUM_TEAMS_DEV"), {OMP_NUM_TEAMS_DEV_}, GOMP_ENV_VAR_SUFFIX_DEV , {&gomp_initial_icvs_dev.nteams_var, false}, &parse_int},
>> +  {ENTRY ("OMP_NUM_TEAMS_ALL"), {OMP_NUM_TEAMS_DEV_}, GOMP_ENV_VAR_SUFFIX_ALL, {&gomp_initial_icvs_all.nteams_var, false}, &parse_int},
>> +  {ENTRY ("OMP_NUM_TEAMS"), {OMP_NUM_TEAMS_DEV_}, GOMP_ENV_VAR_SUFFIX_NONE, {&gomp_initial_icvs_none.nteams_var, false}, &parse_int},
>> +
>> +  {ENTRY ("OMP_DYNAMIC_DEV"), {OMP_DYNAMIC_DEV_}, GOMP_ENV_VAR_SUFFIX_DEV, {&gomp_initial_icvs_dev.dyn_var}, &parse_boolean},
>> +  {ENTRY ("OMP_DYNAMIC_ALL"), {OMP_DYNAMIC_DEV_}, GOMP_ENV_VAR_SUFFIX_ALL, {&gomp_initial_icvs_all.dyn_var}, &parse_boolean},
>> +  {ENTRY ("OMP_DYNAMIC"), {OMP_DYNAMIC_DEV_}, GOMP_ENV_VAR_SUFFIX_NONE, {&gomp_initial_icvs_none.dyn_var}, &parse_boolean},
>> +
>> +  {ENTRY ("OMP_TEAMS_THREAD_LIMIT_DEV"), {OMP_TEAMS_THREAD_LIMIT_DEV_}, GOMP_ENV_VAR_SUFFIX_DEV, {&gomp_initial_icvs_dev.teams_thread_limit_var, false}, &parse_int},
>> +  {ENTRY ("OMP_TEAMS_THREAD_LIMIT_ALL"), {OMP_TEAMS_THREAD_LIMIT_DEV_}, GOMP_ENV_VAR_SUFFIX_ALL, {&gomp_initial_icvs_all.teams_thread_limit_var, false}, &parse_int},
>> +  {ENTRY ("OMP_TEAMS_THREAD_LIMIT"), {OMP_TEAMS_THREAD_LIMIT_DEV_}, GOMP_ENV_VAR_SUFFIX_NONE, {&gomp_initial_icvs_none.teams_thread_limit_var, false}, &parse_int},
>> +
>> +  {ENTRY ("OMP_THREAD_LIMIT_DEV"), {OMP_THREAD_LIMIT_DEV_}, GOMP_ENV_VAR_SUFFIX_DEV, {&gomp_initial_icvs_dev.thread_limit_var, false, (void *) UINT_MAX}, &parse_unsigned_long},
>> +  {ENTRY ("OMP_THREAD_LIMIT_ALL"), {OMP_THREAD_LIMIT_DEV_}, GOMP_ENV_VAR_SUFFIX_ALL, {&gomp_initial_icvs_all.thread_limit_var, false, (void *) UINT_MAX}, &parse_unsigned_long},
>> +  {ENTRY ("OMP_THREAD_LIMIT"), {OMP_THREAD_LIMIT_DEV_}, GOMP_ENV_VAR_SUFFIX_NONE, {&gomp_initial_icvs_none.thread_limit_var, false, (void *) UINT_MAX}, &parse_unsigned_long},
>> +
>> +  {ENTRY ("OMP_NUM_THREADS_DEV"), {OMP_NUM_THREADS_DEV_, OMP_NTHREADS_LIST_DEV, OMP_NTHREADS_LIST_LEN_DEV}, GOMP_ENV_VAR_SUFFIX_DEV, {&gomp_initial_icvs_dev.nthreads_var, &gomp_initial_icvs_dev.nthreads_var_list, &gomp_initial_icvs_dev.nthreads_var_list_len}, &parse_unsigned_long_list},
>> +  {ENTRY ("OMP_NUM_THREADS_ALL"), {OMP_NUM_THREADS_DEV_, OMP_NTHREADS_LIST_DEV, OMP_NTHREADS_LIST_LEN_DEV}, GOMP_ENV_VAR_SUFFIX_ALL, {&gomp_initial_icvs_all.nthreads_var, &gomp_initial_icvs_all.nthreads_var_list, &gomp_initial_icvs_all.nthreads_var_list_len}, &parse_unsigned_long_list},
>> +  {ENTRY ("OMP_NUM_THREADS"), {OMP_NUM_THREADS_DEV_, OMP_NTHREADS_LIST_DEV, OMP_NTHREADS_LIST_LEN_DEV}, GOMP_ENV_VAR_SUFFIX_NONE, {&gomp_initial_icvs_none.nthreads_var, &gomp_initial_icvs_none.nthreads_var_list, &gomp_initial_icvs_none.nthreads_var_list_len}, &parse_unsigned_long_list},
>> +
>> +  {ENTRY ("OMP_PROC_BIND_DEV"), {OMP_PROC_BIND_DEV_, OMP_PROC_BIND_LIST_DEV_, OMP_PROC_BIND_LIST_LEN_DEV_}, GOMP_ENV_VAR_SUFFIX_DEV, {&gomp_initial_icvs_dev.bind_var, &gomp_initial_icvs_dev.bind_var_list, &gomp_initial_icvs_dev.bind_var_list_len}, &parse_bind_var},
>> +  {ENTRY ("OMP_PROC_BIND_ALL"), {OMP_PROC_BIND_DEV_, OMP_PROC_BIND_LIST_DEV_, OMP_PROC_BIND_LIST_LEN_DEV_}, GOMP_ENV_VAR_SUFFIX_ALL, {&gomp_initial_icvs_all.bind_var, &gomp_initial_icvs_all.bind_var_list, &gomp_initial_icvs_all.bind_var_list_len}, &parse_bind_var},
>> +  {ENTRY ("OMP_PROC_BIND"), {OMP_PROC_BIND_DEV_, OMP_PROC_BIND_LIST_DEV_, OMP_PROC_BIND_LIST_LEN_DEV_}, GOMP_ENV_VAR_SUFFIX_NONE, {&gomp_initial_icvs_none.bind_var, &gomp_initial_icvs_none.bind_var_list, &gomp_initial_icvs_none.bind_var_list_len}, &parse_bind_var},
>> +
>> +  {ENTRY ("OMP_MAX_ACTIVE_LEVELS_DEV"), {OMP_MAX_ACTIVE_LEVELS_DEV_}, GOMP_ENV_VAR_SUFFIX_DEV, {&gomp_initial_icvs_dev.max_active_levels_var, (void *) true, (void *) gomp_supported_active_levels}, &parse_unsigned_long},
>> +  {ENTRY ("OMP_MAX_ACTIVE_LEVELS_ALL"), {OMP_MAX_ACTIVE_LEVELS_DEV_}, GOMP_ENV_VAR_SUFFIX_ALL, {&gomp_initial_icvs_all.max_active_levels_var, (void *) true, (void *) gomp_supported_active_levels}, &parse_unsigned_long},
>> +  {ENTRY ("OMP_MAX_ACTIVE_LEVELS"), {OMP_MAX_ACTIVE_LEVELS_DEV_}, GOMP_ENV_VAR_SUFFIX_NONE, {&gomp_initial_icvs_none.max_active_levels_var, (void *) true, (void *) gomp_supported_active_levels}, &parse_unsigned_long},
>> +
>> +  {ENTRY ("OMP_WAIT_POLICY_DEV"), {OMP_WAIT_POLICY_DEV_}, GOMP_ENV_VAR_SUFFIX_DEV, {&gomp_initial_icvs_dev.wait_policy}, &parse_wait_policy},
>> +  {ENTRY ("OMP_WAIT_POLICY_ALL"), {OMP_WAIT_POLICY_DEV_}, GOMP_ENV_VAR_SUFFIX_ALL, {&gomp_initial_icvs_all.wait_policy}, &parse_wait_policy},
>> +  {ENTRY ("OMP_WAIT_POLICY"), {OMP_WAIT_POLICY_DEV_}, GOMP_ENV_VAR_SUFFIX_NONE, {&gomp_initial_icvs_none.wait_policy}, &parse_wait_policy},
>> +
>> +  {ENTRY ("OMP_STACKSIZE_DEV"), {OMP_STACKSIZE_DEV_}, GOMP_ENV_VAR_SUFFIX_DEV, {&gomp_initial_icvs_dev.stacksize}, &parse_stacksize},
>> +  {ENTRY ("OMP_STACKSIZE_ALL"), {OMP_STACKSIZE_DEV_}, GOMP_ENV_VAR_SUFFIX_ALL, {&gomp_initial_icvs_all.stacksize}, &parse_stacksize},
>> +  {ENTRY ("OMP_STACKSIZE"), {OMP_STACKSIZE_DEV_}, GOMP_ENV_VAR_SUFFIX_NONE, {&gomp_initial_icvs_none.stacksize}, &parse_stacksize},
>> +
>> +  {ENTRY ("OMP_CANCELLATION"), {}, GOMP_ENV_VAR_SUFFIX_NONE, {&gomp_cancel_var}, &parse_boolean},
>> +  {ENTRY ("OMP_DISPLAY_AFFINITY"), {}, GOMP_ENV_VAR_SUFFIX_NONE, {&gomp_display_affinity_var}, &parse_boolean},
>> +  {ENTRY ("OMP_TARGET_OFFLOAD"), {}, GOMP_ENV_VAR_SUFFIX_NONE, {&gomp_target_offload_var}, &parse_target_offload},
>> +  {ENTRY ("OMP_MAX_TASK_PRIORITY"), {}, GOMP_ENV_VAR_SUFFIX_NONE, {&gomp_max_task_priority_var, (void *) true}, &parse_int},
>> +  {ENTRY ("OMP_ALLOCATOR"), {}, GOMP_ENV_VAR_SUFFIX_NONE, {&gomp_def_allocator}, &parse_allocator},
>> +  {ENTRY ("OMP_DEFAULT_DEVICE"), {OMP_DEFAULT_DEVICE_DEV_}, GOMP_ENV_VAR_SUFFIX_NONE, {&gomp_initial_icvs_none.default_device_var, (void *) true}, &parse_int},
>> +};
>
> This is very large, why do you need 3 entries per most of the env vars?
> Just one would be enough, with just some flag whether it accepts any
> suffixes or not.

The tables have been optimized now. Only one entry per ICV and removed some parts.

> Lines are too long, they'd need wrapping.
> I think the coding conventions are space after { and space before }.

Both changed.

>
>> +static bool
>> +get_device_num (char *env, int *dev_num, int *dev_num_len)
>> +{
>> +  int pos = 0;
>> +  char dev_num_str[INT_MAX_STR_LEN+1];
>> +
>> +  *dev_num_len = 0;
>> +  *dev_num = -1;
>> +  if (env == NULL)
>> +    return false;
>> +
>> +  while (pos < INT_MAX_STR_LEN)
>> +    {
>> +      if (env[pos] == '\0' || env[pos] == '=')
>> +    break;
>> +      dev_num_str[pos] = env[pos];
>> +      pos++;
>> +    }
>> +
>> +  if (env[pos] != '=' || (dev_num_str[0] == '0' && pos > 1))
>> +    return false;
>> +
>> +  dev_num_str[pos] = '\0';
>> +  *dev_num = (int) strtoul (dev_num_str, 0, 10);
>> +  *dev_num_len = pos;
>
> Why do you copy the chars to a separate stack buffer?
> strtoul will stop on anything that isn't a number.
> So, just passing it second argument and verifying that
> it points to '=' ('\0' wouldn't be valid) would be good enough
> (or perhaps also verying it doesn't point to the start pointer
> in case caller hasn't checked yet if it starts with a digit).

Thanks for this hint. That makes it much simpler. Changed.

> Also, the (int) cast means it throws away important information,
> we'd treat say
> 8589934593
> on 64-bit arches as 1,  I think we want to just ignore it.
> Also, we should ignore negative values (though, for 5.2 there
> is a question if OMP_NUM_THREADS_DEV_-1=5 is valid or not and
> redundant with OMP_NUM_THREADS_DEV_4=5 (if omp_get_num_devices() == 5)
> or OMP_NUM_THREADS and which one wins.

Negative device numbers are rejected now. Btw. it seems that defining
environment variables like "OMP_NUM_THREADS_DEV_-1" is not always valid due to
"-" (invalid identifier name).

>
>> +  return true;
>> +}
>> +
>> +/* Helper function for initialize_env to add a device specific ICV value
>> +   to gomp_initial_icv_dev_list.  */
>> +
>> +static void
>> +add_device_specific_icv (int dev_num, int icv_code, void *value)
>> +{
>> +  struct gomp_icv_list *list = gomp_initial_icv_dev_list;
>> +  while (list != NULL && list->device_num != dev_num)
>> +    list = list->next;
>> +
>> +  if (list == NULL)
>> +    {
>> +      list =
>> +    (struct gomp_icv_list *) gomp_malloc (sizeof (struct gomp_icv_list));
>
> Formatting, = can't be at the end of line.
>
>> +static unsigned char
>> +get_icv_flag (unsigned char flag_var)
>> +{
>> +  switch (flag_var)
>> +    {
>> +    case OMP_NUM_TEAMS_DEV_:
>> +      return gomp_initial_icv_flags.nteams_var;
>> +    case OMP_SCHEDULE_DEV_:
>> +      return gomp_initial_icv_flags.run_sched_var;
>> +    case OMP_SCHEDULE_CHUNK_SIZE_DEV_:
>> +      return gomp_initial_icv_flags.run_sched_chunk_size;
>> +    case OMP_DYNAMIC_DEV_:
>> +      return gomp_initial_icv_flags.dyn_var;
>> +    case OMP_TEAMS_THREAD_LIMIT_DEV_:
>> +      return gomp_initial_icv_flags.teams_thread_limit_var;
>> +    case OMP_THREAD_LIMIT_DEV_:
>> +      return gomp_initial_icv_flags.thread_limit_var;
>> +    case OMP_NUM_THREADS_DEV_:
>> +      return gomp_initial_icv_flags.nthreads_var;
>> +    case OMP_NTHREADS_LIST_DEV:
>> +      return gomp_initial_icv_flags.nthreads_var_list;
>> +    case OMP_NTHREADS_LIST_LEN_DEV:
>> +      return gomp_initial_icv_flags.nthreads_var_list_len;
>> +    case OMP_PROC_BIND_DEV_:
>> +      return gomp_initial_icv_flags.bind_var;
>> +    case OMP_PROC_BIND_LIST_DEV_:
>> +      return gomp_initial_icv_flags.bind_var_list;
>> +    case OMP_PROC_BIND_LIST_LEN_DEV_:
>> +      return gomp_initial_icv_flags.bind_var_list_len;
>> +    case OMP_MAX_ACTIVE_LEVELS_DEV_:
>> +      return gomp_initial_icv_flags.max_active_levels_var;
>> +    case OMP_WAIT_POLICY_DEV_:
>> +      return gomp_initial_icv_flags.wait_policy;
>> +    case OMP_STACKSIZE_DEV_:
>> +      return gomp_initial_icv_flags.stacksize;
>> +    case OMP_DEFAULT_DEVICE_DEV_:
>> +      return gomp_initial_icv_flags.default_device_var;
>> +    default:
>> +      return OMP_NONE;
>
> Doesn't this function return a bitmask of GOMP_ENV_VAR_SUFFIX_WHATEVER
> values?  OMP_NONE isn't one of them, shouldn't it return 0 instead?
> But more importantly, wouldn't it be easier if the icv_flags was just
> an array indexed by flag_var?  You don't need a large switch to handle
> setting or getting it then.  As you just want 4 bits per flag instead of 8,
> you could index it by flag_var >> 1 and for flag_var & 1 shift right or left
> by 4.

The ICV flags are now defined as uint32_t. This is enough to store flags for our
current 21 ICVs. The flags for _DEV, _DEV_X, _ALL and no suffix are now
separated as we have the flags variable for each item of the initial icv list.

>
>>   static void __attribute__((constructor))
>>   initialize_env (void)
>>   {
>> -  unsigned long thread_limit_var;
>> -  unsigned long max_active_levels_var;
>> +  extern char **environ;
>> +  char **env;
>> +  int omp_var, dev_num = 0, dev_num_len = 0, int_value, k;
>> +  bool bool_value, ignore = false;
>> +  char *env_val;
>>
>>     /* Do a compile time check that mkomp_h.pl did good job.  */
>>     omp_check_defines ();
>>
>> -  parse_schedule ();
>> -  parse_boolean ("OMP_DYNAMIC", &gomp_global_icv.dyn_var);
>> -  parse_boolean ("OMP_CANCELLATION", &gomp_cancel_var);
>> -  parse_boolean ("OMP_DISPLAY_AFFINITY", &gomp_display_affinity_var);
>> -  parse_int ("OMP_DEFAULT_DEVICE", &gomp_global_icv.default_device_var, true);
>> -  parse_target_offload ("OMP_TARGET_OFFLOAD", &gomp_target_offload_var);
>> -  parse_int ("OMP_MAX_TASK_PRIORITY", &gomp_max_task_priority_var, true);
>> -  gomp_def_allocator = parse_allocator ();
>> -  if (parse_unsigned_long ("OMP_THREAD_LIMIT", &thread_limit_var, false))
>> -    {
>> -      gomp_global_icv.thread_limit_var
>> -    = thread_limit_var > INT_MAX ? UINT_MAX : thread_limit_var;
>> -    }
>> -  parse_int_secure ("GOMP_DEBUG", &gomp_debug_var, true);
>>   #ifndef HAVE_SYNC_BUILTINS
>>     gomp_mutex_init (&gomp_managed_threads_lock);
>>   #endif
>>     gomp_init_num_threads ();
>>     gomp_available_cpus = gomp_global_icv.nthreads_var;
>> -  if (!parse_unsigned_long_list ("OMP_NUM_THREADS",
>> -                             &gomp_global_icv.nthreads_var,
>> -                             &gomp_nthreads_var_list,
>> -                             &gomp_nthreads_var_list_len))
>> -    gomp_global_icv.nthreads_var = gomp_available_cpus;
>> -  parse_int ("OMP_NUM_TEAMS", &gomp_nteams_var, false);
>> -  parse_int ("OMP_TEAMS_THREAD_LIMIT", &gomp_teams_thread_limit_var, false);
>> -  bool ignore = false;
>> -  if (parse_bind_var ("OMP_PROC_BIND",
>> -                  &gomp_global_icv.bind_var,
>> -                  &gomp_bind_var_list,
>> -                  &gomp_bind_var_list_len)
>> +
>> +  for (env = environ; *env != 0; env++)
>> +    {
>> +      if (!startswith (*env, "OMP_"))
>> +    continue;
>
> While the above is certainly good and quickly skips non-OpenMP env vars,
> I think
>
>> +
>> +      for (omp_var = 0; omp_var < OMP_DEV_VAR_CNT; omp_var++)
>> +    {
>> +      if (startswith (*env, envvars_dev[omp_var].name))
>
> walking 36 entries for each OMP_ env var and doing strncmp for each is
> expensive.
>
> Wouldn't it be better to just walk the name once, find out the suffix in
> there and the length of the part before it too,
> just a simple loop, stopping at '=' and when seeing '_', check if followed
> by "ALL=", "DEV=" or "DEV_" followed by digit.  That will determine the
> kind and length of the env var name without the suffixes, so then
> you can just walk the much shortened table with just 16 entries now
> and can skip entries which don't have the computed length (the table
> includes name_len, so start by
>    if (envvars[omp_var].name_len != name_len)
>      continue;
>    if (memcmp (*env + strlen ("OMP_"), envvars[omp_var].name,
>             envvars[omp_var].name_len) != 0)
>      continue;

That's a good point. Together with the "envvars" table the parsing has been
optimized. Similarly to you proposal I just use a flag in the table and parse
the environment variable variants successively. First the basic variable name
(e.g. OMP_NUM_TEAMS) and then checking whether "=", "_DEV=", "_ALL=", or "_DEV_"
is following if allowed according to the table's flag. (Only) for a directly
following "=" a check is not necessary because host variants are always allowed
(thus GOMP_ENV_VAR_SUFFIX_NONE is also omitted in the table).

>
>> --- a/libgomp/libgomp.h
>> +++ b/libgomp/libgomp.h
>> @@ -454,6 +454,24 @@ struct gomp_team_state
>>
>>   struct target_mem_desc;
>>
>> +#define OMP_NONE 0
>> +#define OMP_NUM_TEAMS_DEV_ 1
>> +#define OMP_SCHEDULE_DEV_ 2
>> +#define OMP_SCHEDULE_CHUNK_SIZE_DEV_ 3
>> +#define OMP_DYNAMIC_DEV_ 4
>> +#define OMP_TEAMS_THREAD_LIMIT_DEV_ 5
>> +#define OMP_THREAD_LIMIT_DEV_ 6
>> +#define OMP_NUM_THREADS_DEV_ 7
>> +#define OMP_NTHREADS_LIST_DEV 8
>> +#define OMP_NTHREADS_LIST_LEN_DEV 9
>> +#define OMP_PROC_BIND_DEV_ 10
>> +#define OMP_PROC_BIND_LIST_DEV_ 11
>> +#define OMP_PROC_BIND_LIST_LEN_DEV_ 12
>> +#define OMP_MAX_ACTIVE_LEVELS_DEV_ 13
>> +#define OMP_WAIT_POLICY_DEV_ 14
>> +#define OMP_STACKSIZE_DEV_ 15
>> +#define OMP_DEFAULT_DEVICE_DEV_ 16
>
> These aren't constans defined in OpenMP standard, so I think it would
> be better to use different prefixes, say GOMP_ICV_WHATEVER or
> GOMP_ENV_WHATEVER.  OMP_NONE is very much non-descriptive of what
> it means. Why do some defines have _DEV_ suffixes and others _DEV?
They are renamed accordingly.

> It should also be an enum rather than set of defines and I don't see a
> reason for the _DEV* suffixes.

Agreed and changed accordingly.

>
>> +
>>   /* These are the OpenMP 4.0 Internal Control Variables described in
>>      section 2.3.1.  Those described as having one copy per task are
>>      stored within the structure; those described as having one copy
>> @@ -473,6 +491,69 @@ struct gomp_task_icv
>>     struct target_mem_desc *target_data;
>>   };
>>
>> +#define GOMP_ENV_VAR_SUFFIX_UNKNOWN 0
>> +#define GOMP_ENV_VAR_SUFFIX_NONE 1
>> +#define GOMP_ENV_VAR_SUFFIX_DEV 2
>> +#define GOMP_ENV_VAR_SUFFIX_ALL 4
>> +#define GOMP_ENV_VAR_SUFFIX_DEV_X 8
>
> Similarly, make this an enum, and perhaps just GOMP_ENV_SUFFIX_WHATEVER ?

Also changed.

An updated patch is attached and tested again on x86_64-linux with nvptx and
amdgcn offloading without regression.

Marcel
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955