Three patches: affinity cleanup, 8-bit atomics, types cleanup

Runtimes OpenMP
1

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.

  1. atomic8.patch: Adding some 8-bit atomic operations for future use.

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

Thanks!
Terry

aff_disabled.patch (12.4 KB)

atomic8.patch (7.83 KB)

cleanup.patch (2.65 KB)

2

From: "Terry L Wilmarth" <terry.l.wilmarth@intel.com>
To: openmp-dev@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

3

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

Thanks!
Terry