[GitHub] [tvm] echuraev commented on a diff in pull request #11180: [WIP] [OpenCL] Change of OpenCL profiling logic

[GitBox <gi...@apache.org>]

echuraev commented on code in PR #11180:
URL: https://github.com/apache/tvm/pull/11180#discussion_r866715240


##########
src/runtime/opencl/opencl_common.h:
##########
@@ -459,20 +461,10 @@ class OpenCLTimerNode : public TimerNode {
   virtual int64_t SyncAndGetElapsedNanos() { return this->duration; }
   // destructor
   virtual ~OpenCLTimerNode() {
-    if (cl::OpenCLWorkspace::Global()->profiling) {
+    if (cl::OpenCLWorkspace::Global()->IsProfiling(dev_)) {
       // Profiling session ends, recreate clCommandQueue in non-profiling mode
       // This will disable collection of cl_events in case of executing inference after profile
-      OPENCL_CALL(clFlush(cl::OpenCLWorkspace::Global()->GetQueue(dev_)));
-      OPENCL_CALL(clFinish(cl::OpenCLWorkspace::Global()->GetQueue(dev_)));
-      OPENCL_CALL(clReleaseCommandQueue(cl::OpenCLWorkspace::Global()->GetQueue(dev_)));
-
-      cl_int err_code;
-      cl_device_id did = cl::OpenCLWorkspace::Global()->devices[dev_.device_id];
-      auto normal_queue =
-          clCreateCommandQueue(cl::OpenCLWorkspace::Global()->context, did, 0, &err_code);
-      OPENCL_CHECK_ERROR(err_code);
-      cl::OpenCLWorkspace::Global()->queues[dev_.device_id] = normal_queue;
-      cl::OpenCLWorkspace::Global()->profiling = false;
+      recreateCommandQueue(false);

Review Comment:
   It's up to you, just my opinion that this method name with boolean argument might confuse the developers, and it will be necessary to read the implementation of this method.
   I would suggest moving the if condition from line 464 into this method and rename it to `enableProfiling(bool)`. Internally, we will detect the current state of the queue and recreate command queue when it is necessary.



##########
src/runtime/opencl/opencl_common.h:
##########
@@ -276,6 +274,20 @@ class OpenCLWorkspace : public DeviceAPI {
         << "Invalid OpenCL device_id=" << dev.device_id;
     return events[dev.device_id];
   }
+  // is current clCommandQueue in profiling mode
+  bool IsProfiling(Device dev) {
+    ICHECK(IsOpenCLDevice(dev));
+    this->Init();
+    ICHECK(dev.device_id >= 0 && static_cast<size_t>(dev.device_id) < queues.size())
+        << "Invalid OpenCL device_id=" << dev.device_id;
+    cl_command_queue queue = queues[dev.device_id];

Review Comment:
   Use `GetQueue` method.



##########
src/runtime/opencl/opencl_common.h:
##########
@@ -485,6 +477,26 @@ class OpenCLTimerNode : public TimerNode {
  private:
   int64_t duration;
   Device dev_;
+
+  void recreateCommandQueue(bool profiling) {
+    cl_command_queue_properties prop;
+    if (profiling) {
+      prop = CL_QUEUE_PROFILING_ENABLE;
+    } else {
+      prop = 0;
+    }
+
+    OPENCL_CALL(clFlush(cl::OpenCLWorkspace::Global()->GetQueue(dev_)));
+    OPENCL_CALL(clFinish(cl::OpenCLWorkspace::Global()->GetQueue(dev_)));
+    OPENCL_CALL(clReleaseCommandQueue(cl::OpenCLWorkspace::Global()->GetQueue(dev_)));

Review Comment:
   nit: You call `cl::OpenCLWorkspace::Global()->GetQueue(dev_)` three times. Probably creating local variable for queue will do this code more readable.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org