[GitHub] [tvm] areusch commented on a change in pull request #7197: [Fix][Autoscheduler] Costmodel enhancement & bug fix for graph debug runtime

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

areusch commented on a change in pull request #7197:
URL: https://github.com/apache/tvm/pull/7197#discussion_r551452806



##########
File path: src/runtime/graph/debug/graph_runtime_debug.cc
##########
@@ -153,9 +153,10 @@ class GraphRuntimeDebug : public GraphRuntime {
     const TVMContext& ctx = data_entry_[entry_id(index, 0)]->ctx;
     TVMSynchronize(ctx.device_type, ctx.device_id, nullptr);
     auto op_tend = std::chrono::high_resolution_clock::now();
-    double op_duration =
-        std::chrono::duration_cast<std::chrono::duration<double> >(op_tend - op_tbegin).count();
-    return op_duration;
+    double op_duration_us =
+        std::chrono::duration_cast<std::chrono::duration<double> >(op_tend - op_tbegin).count() *

Review comment:
       ah...I think this was an incomplete migration in I forgot to fix when forward-porting. I think we should also either adjust [src/runtime/crt/common/crt_runtime_api.c](https://github.com/apache/tvm/blob/0230ddf3436f82a57719a8761fcccf39626ee2be/src/runtime/crt/common/crt_runtime_api.c#L487) or update the layers above this to respect seconds. I personally think seconds makes more sense as a unit for time expressed in double, but we should make that change in a separate PR. 
   
   is this change needed to make your PR pass CI? if not, perhaps we should split into another or at least adjust both locations?




----------------------------------------------------------------
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.

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