Five smaller patches

Hello,

We have another set of small patches.

kmp_place_threads.patch – enables environment variable KMP_PLACE_THREADS for all non-MIC architectures.

proc_bind_fix.patch – Small fix that sets proc-bind-var to proc_bind_false if affinity is not supported.

kmp_get_aff_max_proc_fix.patch – fixes mistake in the kmp_get_affinity_max_proc() api function, also fixes misspelling error.

enable_parallel_build.patch – enables parallel builds using the Top Level GNU Makefile. Example: $ make compiler=clang jobs=4

never_unload_windows.patch – Pins the libiomp5.dll for the lifetime of application (Windows Only). This is a fix for repeated loading and unloading of libiomp5.dll.

Apply patches with:

patch -p0 < kmp_place_threads.patch

patch -p0 < proc_bind_fix.patch

patch -p0 < kmp_get_aff_max_proc_fix.patch

patch -p0 < enable_parallel_build.patch

patch -p0 < never_unload_windows.patch

– Johnny

kmp_place_threads.patch (5.25 KB)

proc_bind_fix.patch (598 Bytes)

kmp_get_aff_max_proc_fix.patch (927 Bytes)

enable_parallel_build.patch (3.26 KB)

never_unload_windows.patch (1.08 KB)

From: "Jonathan L Peyton" <jonathan.l.peyton@intel.com>
To: openmp-dev@dcs-maillist2.engr.illinois.edu
Sent: Wednesday, January 28, 2015 4:42:02 PM
Subject: [Openmp-dev] Five smaller patches

Hello,

We have another set of small patches.

kmp_place_threads.patch – enables environment variable
KMP_PLACE_THREADS for all non-MIC architectures.

Any reason not to make these variables int, instead of unsigned int, and thus avoid the additional casts you're adding?

proc_bind_fix.patch – Small fix that sets proc-bind-var to
proc_bind_false if affinity is not supported.

LGTM.

kmp_get_aff_max_proc_fix.patch – fixes mistake in the
kmp_get_affinity_max_proc() api function, also fixes misspelling
error.

LGTM (please commit spelling fix separately)

enable_parallel_build.patch – enables parallel builds using the Top
Level GNU Makefile. Example: $ make compiler=clang jobs=4

This seems to be doing two things:
- Adding the jobs variable
- Fixing the Fortran module dependencies (and some pdb file thing?)

Both of these look fine, but please commit them separately.

never_unload_windows.patch – Pins the libiomp5.dll for the lifetime
of application (Windows Only). This is a fix for repeated loading
and unloading of libiomp5.dll.

Why are you doing this only if GUIDEDLL_EXPORTS is defined?

-Hal

I have committed 3 (split into 5) patches with svn revisions:

- enable_parallel_build.patch split into two:
  227447: "adding the jobs variable for parallel build"
  227449: "fixing the Fortran modules dependencies"
- kmp_get_aff_max_proc_fix.patch split into two:
  227450: "fixing mistake in kmp_get_affinity_max_proc() api function"
  227451: "fixing typo in error message"
- proc_bind_fix.patch:
  227454: "fix that sets proc-bind-var to proc_bind_false if affinity is not supported"

Two patches were not committed:

> kmp_place_threads.patch – enables environment variable
> KMP_PLACE_THREADS for all non-MIC architectures.

Any reason not to make these variables int, instead of unsigned int, and thus
avoid the additional casts you're adding?

No actually. This is just coding style issue, functionality will be the same for int type. We can fix this patch as you suggested (undesirable for us because we will have differences in code bases), or make another one later for changing the unsigned to int, or live with unsigned. What do you prefer?

> never_unload_windows.patch – Pins the libiomp5.dll for the lifetime of
> application (Windows Only). This is a fix for repeated loading and
> unloading of libiomp5.dll.

Why are you doing this only if GUIDEDLL_EXPORTS is defined?

The defined GUIDEDLL_EXPORTS means that we are building dynamic library. If it is not defined, then we are building static library, that means the added code would pin not the OpenMP library itself but some other dynamic object in which the static library had been linked in. That is not what we wanted; we only wanted to pin our own.

Thanks,
Andrey

From: "Andrey Churbanov" <Andrey.Churbanov@intel.com>
To: "Hal Finkel" <hfinkel@anl.gov>
Cc: openmp-dev@dcs-maillist2.engr.illinois.edu, "Jonathan L Peyton" <jonathan.l.peyton@intel.com>
Sent: Thursday, January 29, 2015 10:40:52 AM
Subject: RE: [Openmp-dev] Five smaller patches

I have committed 3 (split into 5) patches with svn revisions:

- enable_parallel_build.patch split into two:
  227447: "adding the jobs variable for parallel build"
  227449: "fixing the Fortran modules dependencies"
- kmp_get_aff_max_proc_fix.patch split into two:
  227450: "fixing mistake in kmp_get_affinity_max_proc() api
  function"
  227451: "fixing typo in error message"
- proc_bind_fix.patch:
  227454: "fix that sets proc-bind-var to proc_bind_false if affinity
  is not supported"

Two patches were not committed:

> > kmp_place_threads.patch – enables environment variable
> > KMP_PLACE_THREADS for all non-MIC architectures.
>
> Any reason not to make these variables int, instead of unsigned
> int, and thus
> avoid the additional casts you're adding?

No actually. This is just coding style issue, functionality will be
the same for int type. We can fix this patch as you suggested
(undesirable for us because we will have differences in code bases

No, you'd update your internal code base as well.

),
or make another one later for changing the unsigned to int, or live
with unsigned. What do you prefer?

This is not a big deal, I'm fine with the patch as is; the casting is just a bit unfortunate. I actually value consistency here over the number of casts that might be required. We should look at all similar variables, and try to make them all int or all unsigned int. The fact that some are one and some are the other is something we should try to clean up. Please commit this now, and we'll deal with the general cleanup as follow-up work.

> > never_unload_windows.patch – Pins the libiomp5.dll for the
> > lifetime of
> > application (Windows Only). This is a fix for repeated loading
> > and
> > unloading of libiomp5.dll.
>
> Why are you doing this only if GUIDEDLL_EXPORTS is defined?

The defined GUIDEDLL_EXPORTS means that we are building dynamic
library. If it is not defined, then we are building static library,
that means the added code would pin not the OpenMP library itself
but some other dynamic object in which the static library had been
linked in. That is not what we wanted; we only wanted to pin our
own.

Okay, LGTM. GUIDEDLL_EXPORTS seems to be an unfortunate name to mean "we're building a DLL". Can we change it?

Thanks again,
Hal

OK, remaining two patches committed, svn revisions:

227467: "enables environment variable KMP_PLACE_THREADS for all non-MIC architectures"
227469: "Pin the libiomp5.dll for the lifetime of application, Windows-specific"

I also scheduled the cleanup of the first patch (unsigned --> signed + removing corresponding type casts) that will eventually be propagated to llvm, and we will try to rename the GUIDEDLL_EXPORTS macro which is 15+ years old so it is time to give it a better name.

Thanks,
Andrey

From: "Andrey Churbanov" <Andrey.Churbanov@intel.com>
To: "Hal Finkel" <hfinkel@anl.gov>
Cc: openmp-dev@dcs-maillist2.engr.illinois.edu, "Jonathan L Peyton" <jonathan.l.peyton@intel.com>
Sent: Thursday, January 29, 2015 11:38:47 AM
Subject: RE: [Openmp-dev] Five smaller patches

OK, remaining two patches committed, svn revisions:

227467: "enables environment variable KMP_PLACE_THREADS for all
non-MIC architectures"
227469: "Pin the libiomp5.dll for the lifetime of application,
Windows-specific"

I also scheduled the cleanup of the first patch (unsigned --> signed
+ removing corresponding type casts) that will eventually be
propagated to llvm, and we will try to rename the GUIDEDLL_EXPORTS
macro which is 15+ years old so it is time to give it a better name.

Great, thanks!

-Hal