From 633c07be0276f5a1452a7091ec79e124ce4d2e77 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Samuel=20K=2E=20Guti=C3=A9rrez?= <samuel@lanl.gov>
Date: Thu, 30 Jan 2025 14:21:54 -0700
Subject: [PATCH] Cleanup more Pthread code. (#300)

Signed-off-by: Samuel K. Gutierrez <samuel@lanl.gov>
---
 include/quo-vadis-pthread.h | 11 +++---
 src/quo-vadis-pthread.cc    | 74 ++++++++++++++++++-------------------
 src/qvi-pthread.cc          |  2 +-
 3 files changed, 42 insertions(+), 45 deletions(-)

diff --git a/include/quo-vadis-pthread.h b/include/quo-vadis-pthread.h
index 46955e3..a49a897 100644
--- a/include/quo-vadis-pthread.h
+++ b/include/quo-vadis-pthread.h
@@ -28,7 +28,7 @@ extern "C" {
 #endif
 
 /**
- * Mapping policies types.
+ * Placement (or mapping) policy types.
  */
 // Intel policies (KMP_AFFINITY) are :
 // - disabled: prevents the runtime library from making any affinity-related
@@ -37,6 +37,7 @@ extern "C" {
 // - scatter: threads are distributed as evenly as possible across the entire system.
 //            (opposite of compact).
 // - explicit: threads are placed according to a list of OS proc IDs (required)
+// TODO(skg) Do we need all of these synonyms?
 typedef enum {
   QV_POLICY_PACKED     = 1,
   QV_POLICY_COMPACT    = 1,
@@ -46,10 +47,8 @@ typedef enum {
   QV_POLICY_ALTERNATE  = 3,
   QV_POLICY_CORESFIRST = 3,
   QV_POLICY_SCATTER    = 4,
-  QV_POLICY_CHOOSE     = 5,
-} qv_policy_t;
-// TODO(skg) Consider updating prefix to qv_pthread_ if this is only used for
-// Pthread code.
+  QV_POLICY_CHOOSE     = 5
+} qv_pthread_placement_t;
 
 /**
  * Similar to pthread_create(3).
@@ -97,7 +96,7 @@ int
 qv_pthread_colors_fill(
     int *color_array,
     int array_size,
-    qv_policy_t policy,
+    qv_pthread_placement_t policy,
     int stride,
     int npieces
 );
diff --git a/src/quo-vadis-pthread.cc b/src/quo-vadis-pthread.cc
index 7c4846a..fc3b485 100644
--- a/src/quo-vadis-pthread.cc
+++ b/src/quo-vadis-pthread.cc
@@ -45,8 +45,14 @@ qvi_pthread_start_routine(
     void *arg
 ) {
     qvi_pthread_args *args = (qvi_pthread_args *)arg;
-    // TODO(skg) Check return code.
-    args->scope->bind_push();
+
+    const int rc = args->scope->bind_push();
+    if (qvi_unlikely(rc != QV_SUCCESS)) {
+        qvi_log_error(
+            "An error occurred in bind_push(): {} ({})", rc, qv_strerr(rc)
+        );
+        pthread_exit(nullptr);
+    }
 
     void *const ret = args->th_routine(args->th_routine_argp);
     qvi_delete(&args);
@@ -61,9 +67,9 @@ qv_pthread_scope_split(
     int nthreads,
     qv_scope_t ***subscope
 ) {
-    const bool invld_args = !scope || npieces < 0 || !color_array ||
-                            nthreads < 0 || !subscope;
-    if (qvi_unlikely(invld_args)) {
+    const bool invalid_args = !scope || npieces < 0 || !color_array ||
+                              nthreads < 0 || !subscope;
+    if (qvi_unlikely(invalid_args)) {
         return QV_ERR_INVLD_ARG;
     }
     try {
@@ -143,50 +149,42 @@ int
 qv_pthread_colors_fill(
    int *color_array,
    int array_size,
-   qv_policy_t policy,
+   qv_pthread_placement_t policy,
    int stride,
    int npieces
 ) {
-    int rc = QV_SUCCESS;
-
-    if (stride < 1) return QV_ERR_INVLD_ARG;
-
+    const bool invalid_args = !color_array || array_size < 0 ||
+                              stride < 1 || npieces < 1;
+    if (qvi_unlikely(invalid_args)) return QV_ERR_INVLD_ARG;
+    // TODO(skg) We should use the mapping algorithms in qvi-map for these. The
+    // problem is that its interfaces aren't yet suited for this type of
+    // mapping.
     switch(policy) {
-    case QV_POLICY_SPREAD:
-        {
+        case QV_POLICY_PACKED: {
+            // TODO(skg) This looks more like spread.
+            for(int idx = 0 ; idx < array_size ; idx++){
+                // color_array[idx] = (idx+idx*(stride-1))%(npieces);
+                color_array[idx] = (idx*stride)%(npieces);
+            }
             break;
         }
-
-    case QV_POLICY_DISTRIBUTE:
-    //case QV_POLICY_ALTERNATE:
-    //case QV_POLICY_CORESFIRST:
-        {
-            break;
+        case QV_POLICY_SPREAD: {
+            return QV_ERR_NOT_SUPPORTED;
         }
-
-    case QV_POLICY_SCATTER:
-        {
-            break;
+        case QV_POLICY_DISTRIBUTE: {
+            return QV_ERR_NOT_SUPPORTED;
         }
-
-    case QV_POLICY_CHOOSE:
-        {
-            break;
+        case QV_POLICY_SCATTER: {
+            return QV_ERR_NOT_SUPPORTED;
         }
-
-    case QV_POLICY_PACKED:
-    //case QV_POLICY_COMPACT:
-    //case QV_POLICY_CLOSE:
-    default:
-        {
-            for(int idx = 0 ; idx < array_size ; idx++){
-                //    color_array[idx] = (idx+idx*(stride-1))%(npieces);
-                color_array[idx] = (idx*stride)%(npieces);
-            }
-            break;
+        case QV_POLICY_CHOOSE: {
+            return QV_ERR_NOT_SUPPORTED;
+        }
+        default: {
+            return QV_ERR_INVLD_ARG;
         }
     }
-    return rc;
+    return QV_SUCCESS;
 }
 
 /*
diff --git a/src/qvi-pthread.cc b/src/qvi-pthread.cc
index 9131276..b2ecc21 100644
--- a/src/qvi-pthread.cc
+++ b/src/qvi-pthread.cc
@@ -142,7 +142,7 @@ qvi_pthread_group::call_first_from_pthread_create(
             "An error occurred in m_finish_init_by_all_threads(): {} ({})",
             rc, qv_strerr(rc)
         );
-        throw qvi_runtime_error();
+        pthread_exit(nullptr);
     }
     // Free the provided argument container.
     qvi_delete(&args);