[GitHub] [tvm] areusch commented on a change in pull request #6948: [µTVM] Allow for platform-specific malloc in runtime

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

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



##########
File path: src/runtime/crt/common/crt_runtime_api.c
##########
@@ -315,21 +315,30 @@ int TVMFuncFree(TVMFunctionHandle func) {
   return 0;
 }
 
-tvm_crt_error_t TVMInitializeRuntime(uint8_t* memory_pool, size_t memory_pool_size_bytes,
-                                     size_t page_size_bytes_log2) {
+tvm_crt_error_t TVMInitializeRuntime() {
   int idx;
   tvm_crt_error_t error;
+  void* func_registry_memory;
 
-  error =
-      TVMInitializeGlobalMemoryManager(memory_pool, memory_pool_size_bytes, page_size_bytes_log2);
+  system_lib_handle = kTVMModuleHandleUninitialized;
+
+  DLContext ctx = {kDLCPU, 0};
+  error = TVMPlatformMemoryAllocate(TVM_CRT_GLOBAL_FUNC_REGISTRY_SIZE_BYTES, ctx,
+                                    &func_registry_memory);
   if (error != kTvmErrorNoError) {
     return error;
   }
 
   system_lib_handle = kTVMModuleHandleUninitialized;
 
-  TVMMutableFuncRegistry_Create(&global_func_registry,
-                                vmalloc(TVM_CRT_GLOBAL_FUNC_REGISTRY_SIZE_BYTES),
+  void* registry_backing_memory;
+  error = TVMPlatformMemoryAllocate(TVM_CRT_GLOBAL_FUNC_REGISTRY_SIZE_BYTES, ctx,
+                                    &registry_backing_memory);
+  if (error != kTvmErrorNoError) {
+    return error;

Review comment:
       ah this is a great point @liangfu! i've split this function into two parts:
   1. allocating memory
   2. initializing
   
   in the first part, I added TVMPlatformMemoryFree() calls to any early returns. in the second part, I followed your advice and consolidated the returns at the end. there, I added calls to free the allocated memory on error.
   
   I also audited the rest of the codebase for calls to TVMPlatformMemoryAllocate. I found that the rest of the calls could be split into two categories:
   1. calls where a single TVMPlatformMemoryAllocate is performed and no other errors could be generated before returning the allocated memory. in this case, the caller takes responsibility for the memory.
   2. initialization in the GraphRuntime. If something errors here, we will definitely leak memory right now. since most examples of this bail if a failure happens on initialization, and fixing this is quite an invasive change, I'd propose we defer fixing that to another PR. let me know if you think differently.




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