giuseros commented on a change in pull request #7785:
URL: https://github.com/apache/tvm/pull/7785#discussion_r619541593
##########
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 am not following the last comment. If you store X bytes of memory, you
allocate a block of [X/16]*16 bytes. Then you *additionally* allocate 4 bytes
for the tag. So I think there are two issues:
* Additional 4 bytes of memory per block allocated
* Loss of memory alignment. This can really hurt accelerators that usually
have strong alignment requirements.
For these reasons I would leave it off by default, and only enable it for
debugging. If we want to add a feature to enforce the FIFO ordering in
production I would leave it for a subsequent and more focused PR
About the tests, I tried to follow your suggestion to have a local
`crt_config.h`, but the `stack_allocator.c` is compiled in `libmemory.a` that
includes the default `crt_config.h`.
--
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:
[email protected]
