[Openmp-dev] Five smaller patches

Thu Jan 29 09:38:47 PST 2015

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

> -----Original Message-----
> From: Hal Finkel [mailto:hfinkel at anl.gov]
> Sent: Thursday, January 29, 2015 7:50 PM
> To: Churbanov, Andrey
> Cc: openmp-dev at dcs-maillist2.engr.illinois.edu; Peyton, Jonathan L
> Subject: Re: [Openmp-dev] Five smaller patches
> 
> ----- 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

--------------------------------------------------------------------
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.