[Openmp-dev] Three patches: affinity cleanup, 8-bit atomics, types cleanup

[Wilmarth, Terry L]

Thanks for the review Hal!

1) is not my patch (Johnny's I think), so I'll see if I or Andrey can figure out what the separate pieces are.    I really think a & b, and possibly c as well, are too closely related to separate easily.  Possibly d can be broken out.

2) The 1 in TCR_1 is a number of bytes, so it is the 8-bit version.  I added both R&W for completeness.  Agreed on the needed cleanup.  It's on our radar :)

Thanks!
Terry


-----Original Message-----
From: Hal Finkel [mailto:hfinkel at anl.gov] 
Sent: Sunday, March 08, 2015 1:14 AM
To: Wilmarth, Terry L
Cc: openmp-dev at dcs-maillist2.engr.illinois.edu
Subject: Re: [Openmp-dev] Three patches: affinity cleanup, 8-bit atomics, types cleanup

----- Original Message -----
> From: "Terry L Wilmarth" <terry.l.wilmarth at intel.com>
> To: openmp-dev at dcs-maillist2.engr.illinois.edu
> Sent: Friday, March 6, 2015 4:04:25 PM
> Subject: [Openmp-dev] Three patches: affinity cleanup, 8-bit atomics,	types cleanup
> 
> 
> Hello,
> 
> Three more patches:
> 
> 1) aff_disabled.patch: Some affinity fixes/cleanup:
> 
> a. Getting rid of proc_bind_disabled enum value. The disabled
> affinity functionality is flagged instead solely by
> __kmp_affinity_type = affinity_disabled. This solves the problem if
> a Linux machine doesn't support affinity for whatever reason and
> affinity is disabled then it maps to proc-bind-var to
> proc_bind_false (valid value) instead of proc_bind_disabled (invalid
> value).
> 
> b. Adding KMP_AFFINITY_DISABLE() and KMP_AFFINITY_ENABLE(mask_size)
> to go along with KMP_AFFINITY_CAPABLE(). In the grand scheme of
> things, we want to get rid of as many explicit references to the
> mask size as we can to aid hwloc importing. It’s also more readable.
> 
> c. Unused affinity function __kmp_change_thread_affinity_mask()
> deleted.
> 
> d. The first changes in kmp_affinity.cpp concern an incorrect warning
> that is produced when you use OMP_PLACES=0:num_hardware_threads. The
> warning states that you are trying to use the num_hardware_threads
> thread context when you really aren't. For example, on fxe12lin19
> (hyperthreaded), if you specify OMP_PLACES=0:24, it warns against
> using OS thread 24. But only OS threads 0-23 are actually being
> used. This change prevents the warning from being emitted.

This, LGTM, but please separate it into these pieces when you commit.

> 
> 2) atomic8.patch: Adding some 8-bit atomic operations for future use.

+#define TCR_1(a)            (a)
+#define TCW_1(a,b)          (a) = (b)

Two questions:

 1. Why is TCW_1 added, you don't use it?
 2. Why don't you use the existing TCR_8/TCW_8 macros?

Otherwise, this is fine, although the similarities between the Linux and Win32 implementations is certainly unfortunate. This is a pre-existing issue, but we should probably clean that up too (the only differences seem to be __kmp_compare_and_store32 vs. KMP_COMPARE_AND_STORE_REL32, etc.).

> 
> 3) cleanup.patch: Cleanup of unsigned types to be consistent for all
> topology related variables (requested by Hal Finkel).

LGTM, thanks!

 -Hal

> 
> 
> 
> Thanks!
> Terry
> 
> --
> Terry L. Wilmarth
> terry.l.wilmarth at intel.com 217/403-4251
> Intel/SSG/DPD/TCAR/RAD/Threading Runtimes
> 
> 
> _______________________________________________
> 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