Improve hip_prof_api.h's readability

- Don't pass uint32_t arguments by reference.
- Use nullptr instead of NULL.
- Don't add frivolous typedefs.
- Use correct types when available instead of generic integral types.
- Make all roctracer callbacks extern "C" to prepare for a future change
  that will be removing their declaration from hip_runtime_api.h.
- Rename cb/sem sync and release functions -> reader_lock/writer_lock
  acquire and release.

Change-Id: If203fee077d421a9782fcd34607a413b8c3dcfc8


[ROCm/clr commit: 9f09ca929e]

projects/clr/hipamd/src/hip_intercept.cpp

@@ -29,6 +29,8 @@ api_callbacks_table_t callbacks_table;
 
 extern const std::string& FunctionName(const hipFunction_t f);
 
+extern "C" {
+
 const char* hipKernelNameRef(const hipFunction_t f) { return FunctionName(f).c_str(); }
 
 int hipGetStreamDeviceId(hipStream_t stream) {
@@ -40,28 +42,28 @@ int hipGetStreamDeviceId(hipStream_t stream) {
 }
 
 const char* hipKernelNameRefByPtr(const void* hostFunction, hipStream_t) {
-  if (hostFunction == NULL) {
-    return NULL;
+  if (hostFunction == nullptr) {
+    return nullptr;
   }
   return PlatformState::instance().getStatFuncName(hostFunction);
 }
 
 hipError_t hipRegisterApiCallback(uint32_t id, void* fun, void* arg) {
-  return callbacks_table.set_callback(id, reinterpret_cast<api_callbacks_table_t::fun_t>(fun), arg) ?
+  return callbacks_table.set_callback(static_cast<hip_api_id_t>(id), reinterpret_cast<activity_rtapi_callback_t>(fun), arg) ?
     hipSuccess : hipErrorInvalidValue;
 }
 
 hipError_t hipRemoveApiCallback(uint32_t id) {
-  return callbacks_table.set_callback(id, NULL, NULL) ? hipSuccess : hipErrorInvalidValue;
+  return callbacks_table.set_callback(static_cast<hip_api_id_t>(id), nullptr, nullptr) ? hipSuccess : hipErrorInvalidValue;
 }
 
 hipError_t hipRegisterActivityCallback(uint32_t id, void* fun, void* arg) {
-  return callbacks_table.set_activity(id, reinterpret_cast<api_callbacks_table_t::act_t>(fun), arg) ?
+  return callbacks_table.set_activity(static_cast<hip_api_id_t>(id), reinterpret_cast<activity_sync_callback_t>(fun), arg) ?
     hipSuccess : hipErrorInvalidValue;
 }
 
 hipError_t hipRemoveActivityCallback(uint32_t id) {
-  return callbacks_table.set_activity(id, NULL, NULL) ? hipSuccess : hipErrorInvalidValue;
+  return callbacks_table.set_activity(static_cast<hip_api_id_t>(id), nullptr, nullptr) ? hipSuccess : hipErrorInvalidValue;
 }
 
 hipError_t hipEnableTracing(bool enabled) {
@@ -72,3 +74,5 @@ hipError_t hipEnableTracing(bool enabled) {
 const char* hipApiName(uint32_t id) {
   return hip_api_name(id);
 }
+
+} // extern "C"

projects/clr/hipamd/src/hip_prof_api.h

@@ -22,8 +22,10 @@
 #define HIP_SRC_HIP_PROF_API_H
 
 #include <atomic>
+#include <cassert>
 #include <iostream>
 #include <mutex>
+#include <utility>
 
 #if USE_PROF_API
 #include "hip/amd_detail/hip_prof_str.h"
@@ -34,203 +36,164 @@
   api_callbacks_spawner_t<HIP_API_ID_##CB_ID> __api_tracer; \
   { \
     hip_api_data_t* api_data = __api_tracer.get_api_data_ptr(); \
-    if (api_data != NULL) { \
-      hip_api_data_t& api_data_ref = *api_data; \
-      INIT_CB_ARGS_DATA(CB_ID, api_data_ref); \
+    if (api_data != nullptr) { \
+      INIT_CB_ARGS_DATA(CB_ID, (*api_data)); \
       __api_tracer.call(); \
     } \
   }
 
-static const uint32_t HIP_DOMAIN_ID = ACTIVITY_DOMAIN_HIP_API;
-typedef activity_record_t hip_api_record_t;
-typedef activity_rtapi_callback_t hip_api_callback_t;
-typedef activity_sync_callback_t hip_act_callback_t;
-
 class api_callbacks_table_t {
  public:
-  typedef std::mutex mutex_t;
-
-  typedef hip_api_record_t record_t;
-  typedef hip_api_callback_t fun_t;
-  typedef hip_act_callback_t act_t;
-
-  // HIP API callbacks table
-  struct hip_cb_table_entry_t {
-    volatile std::atomic<bool> sync{false};
-    volatile std::atomic<uint32_t> sem{0};
-    act_t act;
-    void* a_arg;
-    fun_t fun;
-    void* arg;
-  };
-
-  struct hip_cb_table_t {
-    hip_cb_table_entry_t arr[HIP_API_ID_LAST + 1] = {};
-  };
-
   api_callbacks_table_t() = default;
 
-  bool set_activity(uint32_t id, act_t fun, void* arg) {
-    std::lock_guard<mutex_t> lock(mutex_);
-    bool ret = true;
+  bool set_activity(hip_api_id_t id, activity_sync_callback_t function, void* arg) {
+    if (id < HIP_API_ID_FIRST || id > HIP_API_ID_LAST)
+      return false;
 
-    if (id >= HIP_API_ID_FIRST && id <= HIP_API_ID_LAST) {
-      cb_sync(id);
-      /*
-      'fun != nullptr' indicates it is activity register call,
-      increment should happen only once but client is free to call
-      register CB multiple times for same API id hence the check
+    std::lock_guard<std::mutex> lock(writer_mutex_);
 
-      'fun == nullptr' indicates it is de-register call and
-      decrement should happen only once hence the check
-      */
-      if (fun != nullptr) {
-        if (callbacks_table_.arr[id].act == nullptr) {
-          enabled_api_count_++;
-        }
-      } else {
-        if (callbacks_table_.arr[id].act != nullptr) {
-          enabled_api_count_--;
-        }
+    /* 'function != nullptr' indicates it is activity register call,
+       increment should happen only once but client is free to call
+       register CB multiple times for same API id hence the check
+
+       'function == nullptr' indicates it is de-register call and
+       decrement should happen only once hence the check. */
+
+    if (function != nullptr) {
+      if (callbacks_table_[id].activity.first == nullptr) {
+        ++enabled_api_count_;
       }
-      if (enabled_api_count_ > 0) {
-        amd::IS_PROFILER_ON = true;
-      } else {
-        amd::IS_PROFILER_ON = false;
-      }
-      callbacks_table_.arr[id].act = fun;
-      callbacks_table_.arr[id].a_arg = arg;
-      cb_release(id);
     } else {
-      ret = false;
+      if (callbacks_table_[id].activity.first != nullptr) {
+        --enabled_api_count_;
+      }
     }
+    amd::IS_PROFILER_ON = (enabled_api_count_ > 0);
+
+    acquire_writer_lock(id);
+    callbacks_table_[id].activity = { function, arg };
+    release_writer_lock(id);
+
+    return true;
+  }
+
+  bool set_callback(hip_api_id_t id, activity_rtapi_callback_t function, void* arg) {
+    if (id < HIP_API_ID_FIRST || id > HIP_API_ID_LAST)
+      return false;
+
+    std::lock_guard<std::mutex> lock(writer_mutex_);
+
+    acquire_writer_lock(id);
+    callbacks_table_[id].user_callback = { function, arg };
+    release_writer_lock(id);
+
+    return true;
+  }
+
+  auto get_callback_and_activity(hip_api_id_t id) {
+    assert(id >= HIP_API_ID_FIRST && id <= HIP_API_ID_LAST && "invalid callback id");
+    auto& entry = callbacks_table_[id];
+
+    acquire_reader_lock(id);
+    auto ret = std::make_pair(entry.user_callback, entry.activity);
+    release_reader_lock(id);
 
     return ret;
   }
 
-  bool set_callback(uint32_t id, fun_t fun, void* arg) {
-    std::lock_guard<mutex_t> lock(mutex_);
-    bool ret = true;
-
-    if (id >= HIP_API_ID_FIRST && id <= HIP_API_ID_LAST) {
-      cb_sync(id);
-      callbacks_table_.arr[id].fun = fun;
-      callbacks_table_.arr[id].arg = arg;
-      cb_release(id);
-    } else {
-      ret = false;
-    }
-
-    return ret;
-  }
-
-  void set_enabled(const bool& enabled) {
+  void set_enabled(bool enabled) {
     amd::IS_PROFILER_ON = enabled;
   }
 
-  inline hip_cb_table_entry_t& entry(const uint32_t& id) {
-    return callbacks_table_.arr[id];
-  }
-
-  inline void sem_sync(const uint32_t& id) {
-    sem_increment(id);
-    while (entry(id).sync.load() == true) sync_wait(id);
-  }
-
-  inline void sem_release(const uint32_t& id) {
-    sem_decrement(id);
-  }
-
-  inline bool is_enabled() const {
+  bool is_enabled() const {
     return amd::IS_PROFILER_ON;
   }
 
  private:
-  inline void cb_sync(const uint32_t& id) {
-    entry(id).sync.store(true);
-    while (entry(id).sem.load() != 0) {}
-  }
+  void acquire_reader_lock(hip_api_id_t id) {
+    auto& entry = callbacks_table_[id];
 
-  inline void cb_release(const uint32_t& id) {
-    entry(id).sync.store(false);
-  }
+    while (true) {
+      if (entry.reader_count++ == std::numeric_limits<uint32_t>::max())
+        assert(!"reader_count overflow");
 
-  inline void sem_increment(const uint32_t& id) {
-    const uint32_t prev = entry(id).sem.fetch_add(1);
-    if (prev == UINT32_MAX) {
-      std::cerr << "sem overflow id = " << id << std::endl << std::flush;
-      abort();
+      if (!entry.writer_lock) break;
+
+      // A writer owns the lock, decrement the reader count and wait for the
+      // writer to release the lock.
+
+      if (entry.reader_count-- == 0) assert (!"reader_count corrupted");
+      while (entry.writer_lock) {}
     }
   }
 
-  inline void sem_decrement(const uint32_t& id) {
-    const uint32_t prev = entry(id).sem.fetch_sub(1);
-    if (prev == 0) {
-      std::cerr << "sem corrupted id = " << id << std::endl << std::flush;
-      abort();
-    }
+  void release_reader_lock(hip_api_id_t id) {
+    if (callbacks_table_[id].reader_count-- == 0)
+      assert (!"reader_count corrupted");
   }
 
-  void sync_wait(const uint32_t& id) {
-    sem_decrement(id);
-    while (entry(id).sync.load() == true) {}
-    sem_increment(id);
+  void acquire_writer_lock(hip_api_id_t id) {
+    callbacks_table_[id].writer_lock = true;
+    while (callbacks_table_[id].reader_count != 0) {}
   }
 
-  mutex_t mutex_;
-  hip_cb_table_t callbacks_table_;
-  uint32_t enabled_api_count_;
+  void release_writer_lock(hip_api_id_t id) {
+    callbacks_table_[id].writer_lock = false;
+  }
+
+  std::mutex writer_mutex_{};
+  uint32_t enabled_api_count_{0};
+
+  // HIP API callbacks table
+  struct  {
+    std::atomic<bool> writer_lock{false};
+    std::atomic<uint32_t> reader_count{0};
+    std::pair<activity_sync_callback_t, void*> activity;
+    std::pair<activity_rtapi_callback_t, void*> user_callback;
+  } callbacks_table_[HIP_API_ID_LAST + 1]{};
 };
 
 extern api_callbacks_table_t callbacks_table;
 
-template <int cid_>
+template <hip_api_id_t ID>
 class api_callbacks_spawner_t {
  public:
   api_callbacks_spawner_t() :
-    api_data_(NULL)
+    api_data_(nullptr)
   {
-    if (!is_enabled()) return;
+    static_assert(ID >= HIP_API_ID_FIRST && ID <= HIP_API_ID_LAST, "invalid callback id");
+    if (!callbacks_table.is_enabled()) return;
 
-    static_assert(cid_ >= HIP_API_ID_FIRST || cid_ <= HIP_API_ID_LAST, "invalid callback id");
-    callbacks_table.sem_sync(cid_);
-    auto &entry = this->entry(cid_);
-    fun_ = std::make_pair(entry.fun, entry.arg);
-    act_ = std::make_pair(entry.act, entry.a_arg);
-    callbacks_table.sem_release(cid_);
+    std::tie(user_callback_, activity_) = callbacks_table.get_callback_and_activity(ID);
 
-    if (act_.first != NULL) api_data_ = (hip_api_data_t*) act_.first(cid_, NULL, NULL, NULL);
+    if (activity_.first != nullptr)
+      api_data_ = (hip_api_data_t*) activity_.first(ID, nullptr, nullptr, nullptr);
   }
 
   void call() {
-    if (fun_.first != NULL) {
-      fun_.first(HIP_DOMAIN_ID, cid_, api_data_, fun_.second);
-      api_data_->phase = ACTIVITY_API_PHASE_EXIT;
-    }
+    if (user_callback_.first != nullptr)
+      user_callback_.first(ACTIVITY_DOMAIN_HIP_API, ID, api_data_, user_callback_.second);
   }
 
   ~api_callbacks_spawner_t() {
-    if (api_data_ != NULL) {
-      if (fun_.first != NULL) fun_.first(HIP_DOMAIN_ID, cid_, api_data_, fun_.second);
-      if (act_.first != NULL) act_.first(cid_, NULL, NULL, act_.second);
-    }
+    if (api_data_ == nullptr)
+      return;
+
+    api_data_->phase = ACTIVITY_API_PHASE_EXIT;
+    if (user_callback_.first != nullptr)
+      user_callback_.first(ACTIVITY_DOMAIN_HIP_API, ID, api_data_, user_callback_.second);
+
+    if (activity_.first != nullptr)
+      activity_.first(ID, nullptr, nullptr, activity_.second);
   }
 
-  hip_api_data_t* get_api_data_ptr() {
+  hip_api_data_t* get_api_data_ptr() const {
     return api_data_;
   }
 
-  bool is_enabled() const {
-    return callbacks_table.is_enabled();
-  }
-
  private:
-  inline api_callbacks_table_t::hip_cb_table_entry_t& entry(const uint32_t& id) {
-    return callbacks_table.entry(id);
-  }
-
-  std::pair<hip_api_callback_t, void *> fun_;
-  std::pair<hip_act_callback_t, void *> act_;
+  std::pair<activity_rtapi_callback_t /* function */, void * /* arg */> user_callback_;
+  std::pair<activity_sync_callback_t /* function */, void * /* arg */> activity_;
   hip_api_data_t* api_data_;
 };
 
@@ -239,8 +202,7 @@ class api_callbacks_spawner_t<HIP_API_ID_NONE> {
  public:
   api_callbacks_spawner_t() {}
   void call() {}
-  hip_api_data_t* get_api_data_ptr() { return NULL; }
-  bool is_enabled() const { return false; }
+  hip_api_data_t* get_api_data_ptr() { return nullptr; }
 };
 
 #else
@@ -249,10 +211,8 @@ class api_callbacks_spawner_t<HIP_API_ID_NONE> {
 
 class api_callbacks_table_t {
  public:
-  typedef void* act_t;
-  typedef void* fun_t;
-  bool set_activity(uint32_t id, act_t fun, void* arg) { return false; }
-  bool set_callback(uint32_t id, fun_t fun, void* arg) { return false; }
+  bool set_activity(hip_api_id_t, activity_sync_callback_t, void*) { return false; }
+  bool set_callback(hip_api_id_t, activity_rtapi_callback_t, void*) { return false; }
 };
 
 #endif