areusch commented on a change in pull request #7785: URL: https://github.com/apache/tvm/pull/7785#discussion_r619326998
########## File path: include/tvm/runtime/crt/stack_allocator.h ########## @@ -0,0 +1,58 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// LINT_C_FILE +#ifndef TVM_RUNTIME_CRT_STACK_ALLOCATOR_H_ +#define TVM_RUNTIME_CRT_STACK_ALLOCATOR_H_ +#include <stddef.h> +#include <stdint.h> + +#include "error_codes.h" + +#define STACK_ALLOCATOR_TAG 0xabcd1234 +#define STACK_ALLOCATOR_TAG_SIZE_BYTES 4 + +/*! Memory alignment for allocator */ + +#ifndef TVM_RUNTIME_ALLOC_ALIGNMENT_BYTES +#define TVM_RUNTIME_ALLOC_ALIGNMENT_BYTES 16 Review comment: why 16? ########## File path: src/runtime/crt/memory/stack_allocator.c ########## @@ -36,6 +41,11 @@ void* StackMemoryManager_Allocate(tvm_workspace_t* tvm_runtime_workspace, int32_ } tvm_crt_error_t StackMemoryManager_Free(tvm_workspace_t* tvm_runtime_workspace, void* ptr) { +#ifdef TVM_CRT_DEBUG + uint32_t tag = *(((uint32_t*)tvm_runtime_workspace->next_alloc) - 1); + uint32_t total_size = (tvm_runtime_workspace->next_alloc - (uint8_t*)ptr); + CHECK_EQ(tag, total_size ^ STACK_ALLOCATOR_TAG, "tag did not match"); Review comment: can you make a more informative error message, since the user is not likely to understand what "tag" means if this condition is not met. also probably good to include `ptr` and maybe `next_alloc`. ########## File path: src/runtime/crt/memory/stack_allocator.c ########## @@ -16,17 +16,22 @@ * specific language governing permissions and limitations * under the License. */ - // LINT_C_FILE - #include <tvm/runtime/crt/stack_allocator.h> +#ifdef TVM_CRT_DEBUG +#include <tvm/runtime/crt/logging.h> +#endif void* StackMemoryManager_Allocate(tvm_workspace_t* tvm_runtime_workspace, int32_t nbytes) { uint32_t offset_bytes = (~nbytes + 1) & (TVM_RUNTIME_ALLOC_ALIGNMENT_BYTES - 1); Review comment: I think this is a bit confusing to read since you use bitwise arithmetic with a negative number--can we write it as: ``` // reserve bytes at the end of the allocation such that next_alloc % TVM_RUNTIME_ALLOC_BYTES == 0. uint32_t total_size_bytes = (nbytes + (TVM_RUNTIME_ALLOC_BYTES - 1)) & (TVM_RUNTIME_ALLOC_BYTES - 1); ``` ########## File path: src/runtime/crt/memory/stack_allocator.c ########## @@ -16,17 +16,22 @@ * specific language governing permissions and limitations * under the License. */ - // LINT_C_FILE - #include <tvm/runtime/crt/stack_allocator.h> +#ifdef TVM_CRT_DEBUG +#include <tvm/runtime/crt/logging.h> +#endif void* StackMemoryManager_Allocate(tvm_workspace_t* tvm_runtime_workspace, int32_t nbytes) { uint32_t offset_bytes = (~nbytes + 1) & (TVM_RUNTIME_ALLOC_ALIGNMENT_BYTES - 1); uint8_t* current_alloc = tvm_runtime_workspace->next_alloc; uint8_t* next_alloc = tvm_runtime_workspace->next_alloc + nbytes + offset_bytes; uint8_t* workspace_end = tvm_runtime_workspace->workspace + tvm_runtime_workspace->workspace_size; - +#ifdef TVM_CRT_DEBUG Review comment: I can see why this is not strictly production code, but I don't think that this should be on only in debug mode. it's pretty easy to get memory corruption in a production system, so i'd propose this is just always-on or disable-able by `#define`. ########## File path: python/tvm/relay/backend/executor_factory.py ########## @@ -14,21 +14,107 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -"""Graph executor factory.""" +"""Executor factory modules.""" +from abc import abstractmethod import warnings + from ..._ffi.base import string_types from ..._ffi.registry import get_global_func from ...runtime import ndarray -class GraphExecutorFactoryModule: +class ExecutorFactoryModule: + """Common interface for executor factory modules + This class describes the common API of different + factory modules + """ + + @abstractmethod + def get_excecutor_config(self): + """Common function to return the internal representation + the executor relies upon to execute the network + """ + raise NotImplementedError + + @abstractmethod + def get_params(self): + """ + Sometimes we want to get params explicitly. Review comment: """Return the compiled parameters.""" these are used in the "typical" case where we execute directly in TVM, so I wouldn't use language here that makes it seems like this is an occasionally-used function. ########## File path: src/tir/transforms/lower_tvm_builtin.cc ########## @@ -184,7 +184,7 @@ class BuiltinLower : public StmtExprMutator { } PrimExpr VisitExpr_(const CallNode* op) final { if (op->op.same_as(builtin::tvm_call_packed())) { - return MakeCallPacked(op, true); + return MakeCallPacked(op, /* use_string_lookup */ true); } else if (op->op.same_as(builtin::tvm_call_cpacked())) { return MakeCallPacked(op, false); Review comment: can you add the `/* use_string_lookup */` here too? ########## File path: tests/python/relay/test_backend_graph_executor.py ########## @@ -148,6 +148,12 @@ def test_plan_memory(): assert len(device_types) == 1 assert len(storage_sizes) == 4 + # Check the specific size of each sid Review comment: might be better to assert on storage_sizes all at once, would provide a better failure message in CI ########## File path: python/tvm/relay/backend/executor_factory.py ########## @@ -14,21 +14,125 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -"""Graph executor factory.""" +"""Executor factory modules.""" +from abc import abstractmethod import warnings + +from tvm import tir + from ..._ffi.base import string_types from ..._ffi.registry import get_global_func from ...runtime import ndarray -class GraphExecutorFactoryModule: +class ExecutorFactoryModule: + """Common interface for executor factory modules + This class describes the common API of different + factory modules + """ + + @abstractmethod + def get_internal_repr(self): + """Common function to return the internal representation Review comment: here I mean that there should be a one-line summary followed by further description if needed. also, please document return types in the docstrings here. -- 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: us...@infra.apache.org
