[OpenMP] Fix misleading warning for OMP_PLACES

[jlpeyton]

Summary

When OMP_PLACES contains an invalid value, the warning informs the user
that the fallback is OMP_PLACES=threads, but the actual internal setting
is OMP_PLACES=cores and is detected as such with KMP_SETTINGS=1.
This patch fixes the warning message to display that OMP_PLACES=cores will be used.

[protze.joachim]

In my quick test, env KMP_SETTINGS=1 OMP_PROC_BIND=spread ./a.out reports OMP_PLACES='cores'. I could not find the code path that sets places to cores in this case, but I think it would be more consistent to have consistent setting in the unset and the invalid case.

Is there a reason to choose threads?

[jlpeyton]

In D95170#2513762, @protze.joachim wrote:

In my quick test, env KMP_SETTINGS=1 OMP_PROC_BIND=spread ./a.out reports OMP_PLACES='cores'. I could not find the code path that sets places to cores in this case, but I think it would be more consistent to have consistent setting in the unset and the invalid case.

Is there a reason to choose threads?

Internally here at Intel we did have it at threads at one point, then somehow it got changed, and this was just to change it back to threads. It would be more consistent to have it default to OMP_PLACES=cores because of the case you mentioned and KMP_AFFINITY defaults to core granularity as well. @AndreyChurbanov , are you ok with keeping it as OMP_PLACES="cores" and just changing the warning message to cores instead?

[AndreyChurbanov]

In D95170#2515674, @jlpeyton wrote:

In D95170#2513762, @protze.joachim wrote:

In my quick test, env KMP_SETTINGS=1 OMP_PROC_BIND=spread ./a.out reports OMP_PLACES='cores'. I could not find the code path that sets places to cores in this case, but I think it would be more consistent to have consistent setting in the unset and the invalid case.

Is there a reason to choose threads?

Internally here at Intel we did have it at threads at one point, then somehow it got changed, and this was just to change it back to threads. It would be more consistent to have it default to OMP_PLACES=cores because of the case you mentioned and KMP_AFFINITY defaults to core granularity as well. @AndreyChurbanov , are you ok with keeping it as OMP_PLACES="cores" and just changing the warning message to cores instead?

Fine with me.

[jlpeyton]

Fixed so that cores is displayed in the warning message.

[tlwilmar]

LGTM

openmp/runtime/src/kmp_settings.cpp

 subplace := num
 subplace := num : num
 subplace := num : num : signed
 signed := num
 signed := + signed
 signed := - signed
 -----------------------------------------------------------------------------*/
 
+// Warning to issue for syntax error during parsing of OMP_PLACES
+static inline void __kmp_omp_places_syntax_warn(const char *var) {
+  KMP_WARNING(SyntaxErrorUsing, var, "\"cores\"");
+}
+
 static int __kmp_parse_subplace_list(const char *var, const char **scan) {
   const char *next;
 
   for (;;) {
     int start, count, stride;
 
     //
     // Read in the starting proc id
     //
     SKIP_WS(*scan);
     if ((**scan < '0') || (**scan > '9')) {
-      KMP_WARNING(SyntaxErrorUsing, var, "\"threads\"");
+      __kmp_omp_places_syntax_warn(var);
       return FALSE;
     }
     next = *scan;
     SKIP_DIGITS(next);
     start = __kmp_str_to_int(*scan, *next);
     KMP_ASSERT(start >= 0);
     *scan = next;
 
     // valid follow sets are ',' ':' and '}'
     SKIP_WS(*scan);
     if (**scan == '}') {
       break;
     }
     if (**scan == ',') {
       (*scan)++; // skip ','
       continue;
     }
     if (**scan != ':') {
-      KMP_WARNING(SyntaxErrorUsing, var, "\"threads\"");
+      __kmp_omp_places_syntax_warn(var);
       return FALSE;
     }
     (*scan)++; // skip ':'
 
     // Read count parameter
     SKIP_WS(*scan);
     if ((**scan < '0') || (**scan > '9')) {
-      KMP_WARNING(SyntaxErrorUsing, var, "\"threads\"");
+      __kmp_omp_places_syntax_warn(var);
       return FALSE;
     }
     next = *scan;
     SKIP_DIGITS(next);
     count = __kmp_str_to_int(*scan, *next);
     KMP_ASSERT(count >= 0);
     *scan = next;
 
     // valid follow sets are ',' ':' and '}'
     SKIP_WS(*scan);
     if (**scan == '}') {
       break;
     }
     if (**scan == ',') {
       (*scan)++; // skip ','
       continue;
     }
     if (**scan != ':') {
-      KMP_WARNING(SyntaxErrorUsing, var, "\"threads\"");
+      __kmp_omp_places_syntax_warn(var);
       return FALSE;
     }
     (*scan)++; // skip ':'
 
     // Read stride parameter
     int sign = +1;
     for (;;) {
       SKIP_WS(*scan);
       if (**scan == '+') {
         (*scan)++; // skip '+'
         continue;
       }
       if (**scan == '-') {
         sign *= -1;
         (*scan)++; // skip '-'
         continue;
       }
       break;
     }
     SKIP_WS(*scan);
     if ((**scan < '0') || (**scan > '9')) {
-      KMP_WARNING(SyntaxErrorUsing, var, "\"threads\"");
+      __kmp_omp_places_syntax_warn(var);
       return FALSE;
     }
     next = *scan;
     SKIP_DIGITS(next);
     stride = __kmp_str_to_int(*scan, *next);
     KMP_ASSERT(stride >= 0);
     *scan = next;
     stride *= sign;
 
     // valid follow sets are ',' and '}'
     SKIP_WS(*scan);
     if (**scan == '}') {
       break;
     }
     if (**scan == ',') {
       (*scan)++; // skip ','
       continue;
     }
 
-    KMP_WARNING(SyntaxErrorUsing, var, "\"threads\"");
+    __kmp_omp_places_syntax_warn(var);
     return FALSE;
   }
   return TRUE;
 }
 
 static int __kmp_parse_place(const char *var, const char **scan) {
   const char *next;
 
   // valid follow sets are '{' '!' and num
   SKIP_WS(*scan);
   if (**scan == '{') {
     (*scan)++; // skip '{'
     if (!__kmp_parse_subplace_list(var, scan)) {
       return FALSE;
     }
     if (**scan != '}') {
-      KMP_WARNING(SyntaxErrorUsing, var, "\"threads\"");
+      __kmp_omp_places_syntax_warn(var);
       return FALSE;
     }
     (*scan)++; // skip '}'
   } else if (**scan == '!') {
     (*scan)++; // skip '!'
     return __kmp_parse_place(var, scan); //'!' has lower precedence than ':'
   } else if ((**scan >= '0') && (**scan <= '9')) {
     next = *scan;
     SKIP_DIGITS(next);
     int proc = __kmp_str_to_int(*scan, *next);
     KMP_ASSERT(proc >= 0);
     *scan = next;
   } else {
-    KMP_WARNING(SyntaxErrorUsing, var, "\"threads\"");
+    __kmp_omp_places_syntax_warn(var);
     return FALSE;
   }
   return TRUE;
 }
 
 static int __kmp_parse_place_list(const char *var, const char *env,
                                   char **place_list) {
   const char *scan = env;
for (;;) {
     if (*scan == '\0') {
       break;
     }
     if (*scan == ',') {
       scan++; // skip ','
       continue;
     }
     if (*scan != ':') {
-      KMP_WARNING(SyntaxErrorUsing, var, "\"threads\"");
+      __kmp_omp_places_syntax_warn(var);
       return FALSE;
     }
     scan++; // skip ':'
 
     // Read count parameter
     SKIP_WS(scan);
     if ((*scan < '0') || (*scan > '9')) {
-      KMP_WARNING(SyntaxErrorUsing, var, "\"threads\"");
+      __kmp_omp_places_syntax_warn(var);
       return FALSE;
     }
     next = scan;
     SKIP_DIGITS(next);
     count = __kmp_str_to_int(scan, *next);
     KMP_ASSERT(count >= 0);
     scan = next;
 
     // valid follow sets are ',' ':' and EOL
     SKIP_WS(scan);
     if (*scan == '\0') {
       break;
     }
     if (*scan == ',') {
       scan++; // skip ','
       continue;
     }
     if (*scan != ':') {
-      KMP_WARNING(SyntaxErrorUsing, var, "\"threads\"");
+      __kmp_omp_places_syntax_warn(var);
       return FALSE;
     }
     scan++; // skip ':'
 
     // Read stride parameter
     int sign = +1;
     for (;;) {
       SKIP_WS(scan);
       if (*scan == '+') {
         scan++; // skip '+'
         continue;
       }
       if (*scan == '-') {
         sign *= -1;
         scan++; // skip '-'
         continue;
       }
       break;
     }
     SKIP_WS(scan);
     if ((*scan < '0') || (*scan > '9')) {
-      KMP_WARNING(SyntaxErrorUsing, var, "\"threads\"");
+      __kmp_omp_places_syntax_warn(var);
       return FALSE;
     }
     next = scan;
     SKIP_DIGITS(next);
     stride = __kmp_str_to_int(scan, *next);
     KMP_ASSERT(stride >= 0);
     scan = next;
     stride *= sign;
 
     // valid follow sets are ',' and EOL
     SKIP_WS(scan);
     if (*scan == '\0') {
       break;
     }
     if (*scan == ',') {
       scan++; // skip ','
       continue;
     }
 
-    KMP_WARNING(SyntaxErrorUsing, var, "\"threads\"");
+    __kmp_omp_places_syntax_warn(var);
     return FALSE;
   }
 
   {
     ptrdiff_t len = scan - env;
     char *retlist = (char *)__kmp_allocate((len + 1) * sizeof(char));
     KMP_MEMCPY_S(retlist, (len + 1) * sizeof(char), env, len * sizeof(char));
     retlist[len] = '\0';
static void __kmp_stg_parse_places(char const *name, char const *value,
   kmp_setting_t **rivals = (kmp_setting_t **)data;
   int rc;
 
   rc = __kmp_stg_check_rivals(name, value, rivals);
   if (rc) {
     return;
   }
 
-  // If OMP_PROC_BIND is not specified but OMP_PLACES is,
-  // then let OMP_PROC_BIND default to true.
-  if (__kmp_nested_proc_bind.bind_types[0] == proc_bind_default) {
-    __kmp_nested_proc_bind.bind_types[0] = proc_bind_true;
-  }
-
-  //__kmp_affinity_num_places = 0;
-
   if (__kmp_match_str("threads", scan, &next)) {
     scan = next;
     __kmp_affinity_type = affinity_compact;
     __kmp_affinity_gran = affinity_gran_thread;
     __kmp_affinity_dups = FALSE;
     kind = "\"threads\"";
   } else if (__kmp_match_str("cores", scan, &next)) {
     scan = next;
#endif
     if (__kmp_affinity_proclist != NULL) {
       KMP_INTERNAL_FREE((void *)__kmp_affinity_proclist);
       __kmp_affinity_proclist = NULL;
     }
     if (__kmp_parse_place_list(name, value, &__kmp_affinity_proclist)) {
       __kmp_affinity_type = affinity_explicit;
       __kmp_affinity_gran = affinity_gran_fine;
       __kmp_affinity_dups = FALSE;
-      if (__kmp_nested_proc_bind.bind_types[0] == proc_bind_default) {
-        __kmp_nested_proc_bind.bind_types[0] = proc_bind_true;
-      }
-    }
+    } else {
+      // Syntax error fallback
+      __kmp_affinity_type = affinity_compact;
+      __kmp_affinity_gran = affinity_gran_core;
+      __kmp_affinity_dups = FALSE;
+    }
     return;
   }
 
   if (__kmp_nested_proc_bind.bind_types[0] == proc_bind_default) {
     __kmp_nested_proc_bind.bind_types[0] = proc_bind_true;
   }
 
   SKIP_WS(scan);

openmp/runtime/test/affinity/omp-places-invalid-syntax.c

+// RUN: %libomp-compile && env KMP_SETTINGS=1 OMP_PLACES=invalid %libomp-run 2>&1 | FileCheck %s
+// CHECK-DAG: Effective settings
+// CHECK: OMP_PLACES=
+// CHECK-SAME: cores
+// REQUIRES: affinity
+
+int main() {
+  #pragma omp parallel
+  {}
+  return 0;
+}