This is an archive of the discontinued LLVM Phabricator instance.

Avoid bad conversion for __kmp_sys_max_nth
ClosedPublic

Authored by dim on Oct 16 2015, 1:14 PM.

Details

Summary

On FreeBSD, compiling openmp trunk results in the following warning,
because PTHREADS_THREADS_MAX is 0xffffffffffffffffU there:

runtime/src/kmp_global.c:117:35: warning: implicit conversion from 'unsigned long' to 'int' changes value from 18446744073709551615 to -1 [-Wconstant-conversion]
int           __kmp_sys_max_nth = KMP_MAX_NTH;
              ~~~~~~~~~~~~~~~~~   ^~~~~~~~~~~
runtime/src/kmp.h:849:34: note: expanded from macro 'KMP_MAX_NTH'
#    define KMP_MAX_NTH          PTHREAD_THREADS_MAX
                                 ^~~~~~~~~~~~~~~~~~~
/usr/include/pthread.h:55:31: note: expanded from macro 'PTHREAD_THREADS_MAX'
#define PTHREAD_THREADS_MAX                     __ULONG_MAX
                                                ^~~~~~~~~~~
/usr/include/x86/_limits.h:61:21: note: expanded from macro '__ULONG_MAX'
#define __ULONG_MAX     0xffffffffffffffff      /* max for an unsigned long */
                        ^~~~~~~~~~~~~~~~~~
1 warning generated.

This patch attempts to avoid it. The first iteration of this patch used
PTHREAD_THREADS_MAX < INT_MAX ? PTHREAD_THREADS_MAX : INT_MAX as the
definition of KMP_MAX_NTH, but Joerg noted that it isn't a constant
expression then, and advised this approach.

Diff Detail

Repository
rL LLVM

Event Timeline

dim updated this revision to Diff 37631.Oct 16 2015, 1:14 PM
dim retitled this revision from to Avoid bad conversion for __kmp_sys_max_nth.
dim updated this object.
dim added reviewers: jlpeyton, joerg.
dim added a subscriber: openmp-commits.
dim added a comment.Oct 16 2015, 1:22 PM

Note that using:

__kmp_sys_max_nth = (int)KMP_MAX_NTH;

will not solve the conversion to -1.

Alternatively, we could #ifdef it in runtime/src/kmp.h, e.g.:

#ifndef KMP_MAX_NTH
#  ifdef PTHREAD_THREADS_MAX
#    if PTHREAD_THREADS_MAX < INT_MAX
#      define KMP_MAX_NTH        PTHREAD_THREADS_MAX
#    else
#      define KMP_MAX_NTH        INT_MAX
#    endif
#  else
#    define KMP_MAX_NTH          (32 * 1024)
#  endif
#endif /* KMP_MAX_NTH */

If you choose this approach, 32Ki seems too small. A current maxed-out SGI is already at 256 sockets which could get you to 256*18*2 = 9216 threads, so over 1/4 of the way to your 32Ki.

You’re not limited to 16bits here, so why not just make this BIG (say 1Gi) which we know is unlikely to be hit in the foreseeable future.

  • Jim

James Cownie <james.h.cownie@intel.com>
SSG/DPD/TCAR (Technical Computing, Analyzers and Runtimes)
Tel: +44 117 9071438

dim added a comment.Oct 19 2015, 3:47 AM

If you choose this approach, 32Ki seems too small. A current maxed-out SGI is already at 256 sockets which could get you to 256*18*2 = 9216 threads, so over 1/4 of the way to your 32Ki.

This review is not about that limit, but about the case where PTHREAD_THREADS_MAX does not fit into an int, which ranges from -2^31 .. 2^31-1 on almost all architectures these days.

That said, I'm not sure where the default 32Ki value came from originally. If some system does not define PTHREAD_THREADS_MAX, it seems pretty safe to assume a not too high limit? Or maybe even just #error out with a message similar to "please define PTHREAD_THREADS_MAX for your system".

jlpeyton edited edge metadata.Oct 19 2015, 9:32 AM

Alternatively, we could #ifdef it in runtime/src/kmp.h, e.g.:

#ifndef KMP_MAX_NTH
#  ifdef PTHREAD_THREADS_MAX
#    if PTHREAD_THREADS_MAX < INT_MAX
#      define KMP_MAX_NTH        PTHREAD_THREADS_MAX
#    else
#      define KMP_MAX_NTH        INT_MAX
#    endif
#  else
#    define KMP_MAX_NTH          (32 * 1024)
#  endif
#endif /* KMP_MAX_NTH */

I would prefer this method. You can change the 32 * 1024 to INT_MAX as well.

dim updated this revision to Diff 37766.Oct 19 2015, 9:49 AM
dim edited edge metadata.

Use alternate method, which only requires changing the header:

  • If PTHREADS_THREADS_MAX is defined, clamp it to INT_MAX
  • If PTHREADS_THREADS_MAX is not defined, use INT_MAX
jlpeyton accepted this revision.Oct 19 2015, 10:03 AM
jlpeyton edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Oct 19 2015, 10:03 AM
This revision was automatically updated to reflect the committed changes.