From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 9035F385354A for ; Tue, 6 Sep 2022 11:51:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9035F385354A Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1662465116; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:in-reply-to:in-reply-to: references:references; bh=WZfSjiyCadLRuv1M03juqxGhkSpDLqKe/yy1oq5khkE=; b=bg+4ocCAFyGxxREt94OukUu+hzWOTNjuifnUqHGMjjKP84nnj3D6MZrked81tND+3wyVO3 kCj4oBTKgTZVoSL9ByCGneZGWjHnYrJCgl3UNkvSgdUHtA4wBh/QiHKvewNxvoOFQrheMu EwBiIsMfvdtZXj4NMycEL2d3QfQfiPg= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-125-U9CiqpgFN6WS0Q4yv179Xg-1; Tue, 06 Sep 2022 07:51:53 -0400 X-MC-Unique: U9CiqpgFN6WS0Q4yv179Xg-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id E58FE18A6523; Tue, 6 Sep 2022 11:51:52 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.41]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A19CFC15BB3; Tue, 6 Sep 2022 11:51:52 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 286BpnXN2017487 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Tue, 6 Sep 2022 13:51:49 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 286BpmXX2017486; Tue, 6 Sep 2022 13:51:48 +0200 Date: Tue, 6 Sep 2022 13:51:48 +0200 From: Jakub Jelinek To: Marcel Vollweiler Cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] OpenMP, libgomp: Environment variable syntax extension. Message-ID: Reply-To: Jakub Jelinek References: <392c847d-e798-2be3-a808-6888de6c90cd@codesourcery.com> <73621930-22ce-c3f1-61e0-f15683f8b281@codesourcery.com> <055f7cfb-2849-ba5a-a161-13333e19e538@codesourcery.com> <7dc0eaf0-a3ed-8145-c43a-e9bb063f6acd@codesourcery.com> MIME-Version: 1.0 In-Reply-To: <7dc0eaf0-a3ed-8145-c43a-e9bb063f6acd@codesourcery.com> X-Scanned-By: MIMEDefang 2.85 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-4.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 > +#include > + > +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