| Message ID | 20230308063138.1490431-1-hongyu.wang@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | libgomp: Fix default value of GOMP_SPINCOUNT [PR 109062] | expand |
On Wed, Mar 08, 2023 at 02:31:38PM +0800, Hongyu Wang wrote: > Hi, > > When OMP_WAIT_POLICY is not specified, current implementation will cause > icv flag GOMP_ICV_WAIT_POLICY unset, so global variable wait_policy > will remain its uninitialized value. Set it to -1 when the flag is not > specified to keep GOMP_SPINCOUNT behavior consistent with its description. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > libgomp/ChangeLog: > > PR libgomp/109062 > * env.c (initialize_env): Set wait_policy to -1 if > OMP_WAIT_POLICY is not specified. > * testsuite/libgomp.c-c++-common/pr109062.c: New test. I think the right spot to fix this would be instead in initialize_icvs, change the icvs->wait_policy = 0; in there to icvs->wait_policy = -1; That way it will be the default for all the devices, not just the initial one. Ok for trunk with that change if it works. > diff --git a/libgomp/env.c b/libgomp/env.c > index c41c1f852cc..fa36a8697d6 100644 > --- a/libgomp/env.c > +++ b/libgomp/env.c > @@ -2249,6 +2249,8 @@ initialize_env (void) > wait_policy = none->icvs.wait_policy; > else if (all != NULL && gomp_get_icv_flag (all->flags, GOMP_ICV_WAIT_POLICY)) > wait_policy = all->icvs.wait_policy; > + else > + wait_policy = -1; > > if (!parse_spincount ("GOMP_SPINCOUNT", &gomp_spin_count_var)) > { > diff --git a/libgomp/testsuite/libgomp.c-c++-common/pr109062.c b/libgomp/testsuite/libgomp.c-c++-common/pr109062.c > new file mode 100644 > index 00000000000..5c7c287dafd > --- /dev/null > +++ b/libgomp/testsuite/libgomp.c-c++-common/pr109062.c > @@ -0,0 +1,14 @@ > +/* { dg-do run } */ > + > +#include <omp.h> > +#include <stdlib.h> > + > +int > +main () > +{ > + omp_display_env (1); > + > + return 0; > +} > + > +/* { dg-output ".*\\\[host] GOMP_SPINCOUNT = '300000'.*" { target native } } */ > -- > 2.31.1 Jakub
> I think the right spot to fix this would be instead in initialize_icvs, > change the > icvs->wait_policy = 0; > in there to > icvs->wait_policy = -1; > That way it will be the default for all the devices, not just the > initial one. It doesn't work, for the code that determines value of wait_policy: if (none != NULL && gomp_get_icv_flag (none->flags, GOMP_ICV_WAIT_POLICY)) wait_policy = none->icvs.wait_policy; else if (all != NULL && gomp_get_icv_flag (all->flags, GOMP_ICV_WAIT_POLICY)) wait_policy = all->icvs.wait_policy; gomp_get_icv_flag (none->flags, GOMP_ICV_WAIT_POLICY) returns true only when OMP_WAIT_POLICY is explicitly set, so the initial icvs->wait_policy could not affect the global wait_policy that used to set GOMP_SPINCOUNT.
Hongyu Wang <wwwhhhyyy333@gmail.com> 于2023年3月8日周三 16:07写道： > > > I think the right spot to fix this would be instead in initialize_icvs, > > change the > > icvs->wait_policy = 0; > > in there to > > icvs->wait_policy = -1; > > That way it will be the default for all the devices, not just the > > initial one. > > It doesn't work, for the code that determines value of wait_policy: > > if (none != NULL && gomp_get_icv_flag (none->flags, GOMP_ICV_WAIT_POLICY)) > wait_policy = none->icvs.wait_policy; > else if (all != NULL && gomp_get_icv_flag (all->flags, GOMP_ICV_WAIT_POLICY)) > wait_policy = all->icvs.wait_policy; > > gomp_get_icv_flag (none->flags, GOMP_ICV_WAIT_POLICY) returns true only when > OMP_WAIT_POLICY is explicitly set, so the initial icvs->wait_policy > could not affect the global wait_policy that used to set > GOMP_SPINCOUNT. Also the global variable wait_policy here is only used for setting spin_count related values that do not belong to any ICV, so there is no need to set icvs->wait_policy since for OMP_WAIT_POLICY_(DEV|ALL) itself only has value 0 for passive and value 1 for active.
On Wed, Mar 08, 2023 at 04:21:46PM +0800, Hongyu Wang wrote: > Hongyu Wang <wwwhhhyyy333@gmail.com> 于2023年3月8日周三 16:07写道： > > > > > I think the right spot to fix this would be instead in initialize_icvs, > > > change the > > > icvs->wait_policy = 0; > > > in there to > > > icvs->wait_policy = -1; > > > That way it will be the default for all the devices, not just the > > > initial one. > > > > It doesn't work, for the code that determines value of wait_policy: > > > > if (none != NULL && gomp_get_icv_flag (none->flags, GOMP_ICV_WAIT_POLICY)) > > wait_policy = none->icvs.wait_policy; > > else if (all != NULL && gomp_get_icv_flag (all->flags, GOMP_ICV_WAIT_POLICY)) > > wait_policy = all->icvs.wait_policy; > > > > gomp_get_icv_flag (none->flags, GOMP_ICV_WAIT_POLICY) returns true only when > > OMP_WAIT_POLICY is explicitly set, so the initial icvs->wait_policy > > could not affect the global wait_policy that used to set > > GOMP_SPINCOUNT. > > Also the global variable wait_policy here is only used for setting > spin_count related values that do not > belong to any ICV, so there is no need to set icvs->wait_policy since > for OMP_WAIT_POLICY_(DEV|ALL) > itself only has value 0 for passive and value 1 for active. Seems for many ICVs the default values are done through gomp_default_icv_values, but that doesn't cover wait_policy. For other vars, the defaults are provided through just initializers of those vars on the var definitions, e.g.: char *gomp_affinity_format_var = "level %L thread %i affinity %A"; So, I'd do the initialize_icvs change and change static int wait_policy; to static int wait_policy = -1; Jakub
> Seems for many ICVs the default values are done through > gomp_default_icv_values, but that doesn't cover wait_policy. > For other vars, the defaults are provided through just initializers of > those vars on the var definitions, e.g.: > char *gomp_affinity_format_var = "level %L thread %i affinity %A"; > So, I'd do the initialize_icvs change and change > static int wait_policy; > to > static int wait_policy = -1; Agreed, here is the updated patch, ok for trunk? When OMP_WAIT_POLICY is not specified, current implementation will cause icv flag GOMP_ICV_WAIT_POLICY unset, so global variable wait_policy will remain its uninitialized value. Initialize it to -1 to make GOMP_SPINCOUNT behavior consistent with its description. libgomp/ChangeLog: PR libgomp/109062 * env.c (wait_policy): Initialize to -1. (initialize_icvs): Initialize icvs->wait_policy to -1. * testsuite/libgomp.c-c++-common/pr109062.c: New test. --- libgomp/env.c | 4 ++-- libgomp/testsuite/libgomp.c-c++-common/pr109062.c | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) create mode 100644 libgomp/testsuite/libgomp.c-c++-common/pr109062.c diff --git a/libgomp/env.c b/libgomp/env.c index c41c1f852cc..e7a035b593c 100644 --- a/libgomp/env.c +++ b/libgomp/env.c @@ -124,7 +124,7 @@ int goacc_default_dims[GOMP_DIM_MAX]; #ifndef LIBGOMP_OFFLOADED_ONLY -static int wait_policy; +static int wait_policy = -1; static unsigned long stacksize = GOMP_DEFAULT_STACKSIZE; static void @@ -1981,7 +1981,7 @@ initialize_icvs (struct gomp_initial_icvs *icvs) icvs->bind_var = gomp_default_icv_values.bind_var; icvs->nteams_var = gomp_default_icv_values.nteams_var; icvs->teams_thread_limit_var = gomp_default_icv_values.teams_thread_limit_var; - icvs->wait_policy = 0; + icvs->wait_policy = -1; } /* Helper function for initialize_env to add a device specific ICV value diff --git a/libgomp/testsuite/libgomp.c-c++-common/pr109062.c b/libgomp/testsuite/libgomp.c-c++-common/pr109062.c new file mode 100644 index 00000000000..5c7c287dafd --- /dev/null +++ b/libgomp/testsuite/libgomp.c-c++-common/pr109062.c @@ -0,0 +1,14 @@ +/* { dg-do run } */ + +#include <omp.h> +#include <stdlib.h> + +int +main () +{ + omp_display_env (1); + + return 0; +} + +/* { dg-output ".*\\\[host] GOMP_SPINCOUNT = '300000'.*" { target native } } */
On Wed, Mar 08, 2023 at 04:54:20PM +0800, Hongyu Wang wrote: > > Seems for many ICVs the default values are done through > > gomp_default_icv_values, but that doesn't cover wait_policy. > > For other vars, the defaults are provided through just initializers of > > those vars on the var definitions, e.g.: > > char *gomp_affinity_format_var = "level %L thread %i affinity %A"; > > So, I'd do the initialize_icvs change and change > > static int wait_policy; > > to > > static int wait_policy = -1; > > Agreed, here is the updated patch, ok for trunk? > > When OMP_WAIT_POLICY is not specified, current implementation will cause > icv flag GOMP_ICV_WAIT_POLICY unset, so global variable wait_policy > will remain its uninitialized value. Initialize it to -1 to make > GOMP_SPINCOUNT behavior consistent with its description. > > libgomp/ChangeLog: > > PR libgomp/109062 > * env.c (wait_policy): Initialize to -1. > (initialize_icvs): Initialize icvs->wait_policy to -1. > * testsuite/libgomp.c-c++-common/pr109062.c: New test. Ok, thanks. Jakub
diff --git a/libgomp/env.c b/libgomp/env.c index c41c1f852cc..fa36a8697d6 100644 --- a/libgomp/env.c +++ b/libgomp/env.c @@ -2249,6 +2249,8 @@ initialize_env (void) wait_policy = none->icvs.wait_policy; else if (all != NULL && gomp_get_icv_flag (all->flags, GOMP_ICV_WAIT_POLICY)) wait_policy = all->icvs.wait_policy; + else + wait_policy = -1; if (!parse_spincount ("GOMP_SPINCOUNT", &gomp_spin_count_var)) { diff --git a/libgomp/testsuite/libgomp.c-c++-common/pr109062.c b/libgomp/testsuite/libgomp.c-c++-common/pr109062.c new file mode 100644 index 00000000000..5c7c287dafd --- /dev/null +++ b/libgomp/testsuite/libgomp.c-c++-common/pr109062.c @@ -0,0 +1,14 @@ +/* { dg-do run } */ + +#include <omp.h> +#include <stdlib.h> + +int +main () +{ + omp_display_env (1); + + return 0; +} + +/* { dg-output ".*\\\[host] GOMP_SPINCOUNT = '300000'.*" { target native } } */