[Openmp-dev] Five smaller patches

[Hal Finkel]

----- Original Message -----
> From: "Andrey Churbanov" <Andrey.Churbanov at intel.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: openmp-dev at dcs-maillist2.engr.illinois.edu, "Jonathan L Peyton" <jonathan.l.peyton at 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

> 
> Thanks,
> Andrey
> 
> > -----Original Message-----
> > From: openmp-dev-bounces at cs.uiuc.edu [mailto:openmp-dev-
> > bounces at cs.uiuc.edu] On Behalf Of Hal Finkel
> > Sent: Thursday, January 29, 2015 2:51 PM
> > To: Peyton, Jonathan L
> > Cc: openmp-dev at dcs-maillist2.engr.illinois.edu
> > Subject: Re: [Openmp-dev] Five smaller patches
> > 
> > ----- Original Message -----
> > > From: "Jonathan L Peyton" <jonathan.l.peyton at intel.com>
> > > To: openmp-dev at 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
> > 
> > >
> > >
> > >
> > > 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
> > >
> > >
> > > _______________________________________________
> > > Openmp-dev mailing list
> > > Openmp-dev at dcs-maillist2.engr.illinois.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/openmp-dev
> > >
> > 
> > --
> > Hal Finkel
> > Assistant Computational Scientist
> > Leadership Computing Facility
> > Argonne National Laboratory
> > 
> > _______________________________________________
> > Openmp-dev mailing list
> > Openmp-dev at dcs-maillist2.engr.illinois.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/openmp-dev
> 
> --------------------------------------------------------------------
> Closed Joint Stock Company Intel A/O
> Registered legal address: Krylatsky Hills Business Park,
> 17 Krylatskaya Str., Bldg 4, Moscow 121614,
> Russian Federation
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory