[Openmp-dev] Five smaller patches

Thu Jan 29 09:40:25 PST 2015

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

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

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