[Bug libgomp/58691] New: OpenMP 4: Surprising results with OMP_PLACES=

[Bug libgomp/58691] New: OpenMP 4: Surprising results with OMP_PLACES=

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58691

            Bug ID: 58691
           Summary: OpenMP 4: Surprising results with OMP_PLACES=
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libgomp
          Assignee: unassigned at gcc dot gnu.org
          Reporter: burnus at gcc dot gnu.org
                CC: jakub at gcc dot gnu.org

I played around with OMP_PLACES and noticed the following oddness:


First, create some OpenMP program:
$  echo "end" > test.f90; gfortran -fopenmp test.f90


Using "threads", "cores" or "sockets", one gets a places result:

$ OMP_PLACES='threads' OMP_DISPLAY_ENV=verbose ./a.out 2>&1 |grep OMP_PLACES
  OMP_PLACES = '{0},{1},{2},{3}'


However, if one specifies a numerical value, one doesn't:

$ OMP_PLACES='{0:2}' OMP_DISPLAY_ENV=verbose ./a.out 2>&1 |grep OMP_PLACES
  OMP_PLACES = ''



The reason - as it turns out - is that one has in libgomp/env.c the following
code:


parse_places_var (const char *name)
{
...
  if (strncasecmp (env, "threads", 7) == 0)
    {
      env += 7;
      level = 1;
    }
...
  if (level)
    {
...
      return gomp_affinity_init_level (level, count, false);
    }

...

  if (gomp_global_icv.bind_var == omp_proc_bind_false)
    return false;

  gomp_places_list_len = 0;
  gomp_places_list = gomp_affinity_alloc (count, false);



Namely: If OMP_PROC_BIND is FALSE, which is the default, the affinity does not
get set.


 * * *

I think in terms of the OpenMP spec, everything is implementation defined.
Nonetheless, the following should match common expectations:


* Both ways of setting OMP_PLACES should act identically.

* If the user has set OMP_PLACES, the default of OMP_PROC_BIND should change to
TRUE.

* If the user has explicitly set OMP_PROC_BIND=FALSE, OMP_PLACES should be
ignored.


* Probably, a few words in libgomp.text's OMP_PROC_BIND or OMP_PLACES to the
interaction of the two wouldn't harm either.

[Bug libgomp/58691] OpenMP 4: Surprising results with OMP_PLACES=

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58691

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2013-10-11
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 30985
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=30985&action=edit
gcc49-pr58691.patch

Looks reasonable, bind-var default value is indeed implementation defined and
setting it to true by default if OMP_PLACES or GOMP_CPU_AFFINITY has been
parsed
into places list is reasonable.  So, how does this look like?

[Bug libgomp/58691] OpenMP 4: Surprising results with OMP_PLACES=

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58691

--- Comment #2 from Tobias Burnus <burnus at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #1)
> Looks reasonable, bind-var default value is indeed implementation defined and
> setting it to true by default if OMP_PLACES or GOMP_CPU_AFFINITY has been
> parsed into places list is reasonable.  So, how does this look like?

Looks good to me.

(Note that that will turn an explicit OMP_PROC_BIND=false into true, which
looks odd - even if I regard it as unlikely that someone wants to have no
proc-binding while setting OMP_PLACES or GOMP_CPU_AFFINITY:

OMP_PLACES='FALSE' OMP_PLACES='{0:4}' OMP_DISPLAY_ENV=verbose ./a.out 2>&1
|grep -E 'OMP_PLACES|OMP_PROC_'
  OMP_PROC_BIND = 'TRUE'
  OMP_PLACES = '{0:4}'

)


In addition, some patch like the following could be used. Side remark:
OMP_PLACES= is not yet documented in libgomp.texi.

--- a/libgomp/libgomp.texi
+++ b/libgomp/libgomp.texi
@@ -1272,3 +1272,4 @@ Specifies whether threads may be moved between
processors. If set to
 @code{TRUE}, OpenMP theads should not be moved, if set to @code{FALSE}
-they may be moved. If undefined, threads may move between processors.
+they may be moved. If undefined, threads may move between processors,
+unless @code{OMP_PLACES} or @code{GOMP_CPU_AFFINITY} is set.

[Bug libgomp/58691] OpenMP 4: Surprising results with OMP_PLACES=

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58691

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Tobias Burnus from comment #2)
> (Note that that will turn an explicit OMP_PROC_BIND=false into true, which
> looks odd - even if I regard it as unlikely that someone wants to have no
> proc-binding while setting OMP_PLACES or GOMP_CPU_AFFINITY:
> 
> OMP_PLACES='FALSE' OMP_PLACES='{0:4}' OMP_DISPLAY_ENV=verbose ./a.out 2>&1
> |grep -E 'OMP_PLACES|OMP_PROC_'
>   OMP_PROC_BIND = 'TRUE'
>   OMP_PLACES = '{0:4}'

That command line doesn't set explicit OMP_PROC_BIND=false, it sets OMP_PLACES
twice, first time to invalid value, but the latter wins.
If you try
OMP_PROC_BIND='FALSE' OMP_PLACES='{0,4}' OMP_DISPLAY_ENV=verbose ./a.out 2>&1
|grep -E 'OMP_PLACES|OMP_PROC_'
you'll see it will print
  OMP_PROC_BIND = 'FALSE'
  OMP_PLACES = ''

I'll go ahead and commit the patch.

> In addition, some patch like the following could be used. Side remark:
> OMP_PLACES= is not yet documented in libgomp.texi.
> 
> --- a/libgomp/libgomp.texi
> +++ b/libgomp/libgomp.texi
> @@ -1272,3 +1272,4 @@ Specifies whether threads may be moved between
> processors. If set to
>  @code{TRUE}, OpenMP theads should not be moved, if set to @code{FALSE}
> -they may be moved. If undefined, threads may move between processors.
> +they may be moved. If undefined, threads may move between processors,
> +unless @code{OMP_PLACES} or @code{GOMP_CPU_AFFINITY} is set.

libgomp.texi needs much more updates for OpenMP 4.0, didn't have time for that
yet (if you have spare cycles, any help with that would be appreciated).
Just saying this when not even mentioning the SPREAD, CLOSE, MASTER (and that
in that case it can be a comma separated list) does not move us much further
though.

[Bug libgomp/58691] OpenMP 4: Surprising results with OMP_PLACES=

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58691

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: jakub
Date: Sat Oct 12 07:52:15 2013
New Revision: 203479

URL: http://gcc.gnu.org/viewcvs?rev=203479&root=gcc&view=rev
Log:
    PR libgomp/58691
    * config/linux/proc.c (gomp_cpuset_popcount): Add unused attribute
    to check variable.
    (gomp_init_num_threads): Move i variable declaration into
    #ifdef CPU_ALLOC_SIZE block.
    * config/linux/affinity.c (gomp_affinity_init_level): Test
    gomp_places_list_len == 0 rather than gomp_places_list == 0
    when checking for topology reading error.
    * team.c (gomp_team_start): Don't handle bind == omp_proc_bind_false.
    * env.c (parse_affinity): Add ignore argument, if true, don't populate
    gomp_places_list, only parse env var and always return false.
    (parse_places_var): Likewise.  Don't check gomp_global_icv.bind_var.
    (initialize_env): Always parse OMP_PLACES and GOMP_CPU_AFFINITY env
    vars, default to OMP_PROC_BIND=true if OMP_PROC_BIND wasn't specified
    and either of these variables were parsed correctly into a places
    list.

Modified:
    trunk/libgomp/ChangeLog
    trunk/libgomp/config/linux/affinity.c
    trunk/libgomp/config/linux/proc.c
    trunk/libgomp/env.c
    trunk/libgomp/team.c