[Openmp-dev] Three patches: affinity cleanup, 8-bit atomics, types cleanup
Wilmarth, Terry L
terry.l.wilmarth at intel.com
Mon Mar 9 09:09:40 PDT 2015
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 :)
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
> 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
> 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()
> 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)
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).
> 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
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory
More information about the Openmp-dev