[uTVM][Runtime] Introduce Virtual Memory Allocator to CRT #5124

liangfu · 2020-03-22T15:45:12Z

As proposed in #5060, this PR brings an implement of a memory container that returns addresses from a single stack.

liangfu · 2020-03-23T06:56:23Z

@tqchen to follow up your suggestion in #5062, the proposed memory container returns addresses from a single stack. Please review.

tqchen · 2020-03-23T15:41:48Z

cc @tmoreau89 @ajtulloch @ZihengJiang @weberlo please help to take a look

tmoreau89

Thanks Liangfu for this PR! I've added some comments

tmoreau89 · 2020-03-23T18:26:44Z

src/runtime/crt/graph_runtime.c

                  if (reader->NextArrayItem(reader)) {
-                    reader->ReadInteger(reader, &(attr->shape[shape_count][5])); ndim++;
+                    reader->ReadInteger(reader, attr_shape_ptr + 5); ndim++;
                    reader->NextArrayItem(reader);


can't this code block be written as a loop?

Nice catch! I successfully converted this into a for loop.

tmoreau89 · 2020-03-23T18:27:29Z

src/runtime/crt/crt_runtime_api.c

@@ -79,7 +79,7 @@ int TVMModGetFunction(TVMModuleHandle mod,
  if (!strcmp(func_name, "load_params")) {
    *out = &TVMGraphRuntime_LoadParams;
  } else {
-    status -1;
+    status = -1;


Should we enable linting on these sources to avoid such typoes?

Actually we do have the CRT sources checked by the cpplint, but it failed to catch the case. As the CRT is growing, I think we might need cppcheck to ensure the compliance to MISRA-C rules, since it contains a very nice checker misra.py .

I caught this with -Wall flag in gcc.

tmoreau89 · 2020-03-23T18:34:16Z

apps/bundle_deploy/Makefile

@@ -37,6 +37,9 @@ demo: $(build_dir)/demo $(build_dir)/bundle.so $(build_dir)/bundle_c.so $(build_
 	TVM_NUM_THREADS=1 $(build_dir)/demo $(build_dir)/bundle.so $(build_dir)/cat.bin
 	TVM_NUM_THREADS=1 $(build_dir)/demo $(build_dir)/bundle_c.so $(build_dir)/cat.bin

+test_crt: $(build_dir)/test $(build_dir)/test_bundle_c.so $(build_dir)/test_data.bin $(build_dir)/test_output.bin
+	TVM_NUM_THREADS=1 $(build_dir)/test $(build_dir)/test_bundle_c.so $(build_dir)/test_data.bin $(build_dir)/test_output.bin $(build_dir)/test_graph.json $(build_dir)/test_params.bin


it would be nice to see some unit tests for the virtual memory allocator; and add those to CI

test coverage has been extended in cpptest, see tests/cpp/crt_memory_test.cc

tqchen · 2020-03-24T01:28:25Z

I wonder if VMem allocator is a bit overkill for cases like micro controller. If we have a constant memory usage and do not do frequent alloc and free, perhaps we will be good by use an arena style allocator(which just allocs but do not free) and frees everything once we are done with the task.

It would be nice to do a quick benchmark of the vmemory allocator vs alternatives(in terms of binary size and efficiency)

liangfu · 2020-03-24T04:01:11Z

I think we do need to reuse workspace memory between convolution layers, and reusing the space requires recycling the space with TVMBackendFreeWorkspace.

tqchen · 2020-03-24T15:37:08Z

The workspace memory could have a different strategy. The way it works is that we create a different arena for workspace, along with a counter.

When a memory is allocated, we allocate memory from the arena, and add the counter
When a memory is de-allocated, we decrease the counter
When the counter goes to zero, we free all the memory.

This will work because all workspace memory are temporal. It also guarantees a constant time allocation

liangfu · 2020-03-26T10:06:16Z

I agree we definitely need an arena style allocator, but can we introduce that in a separate PR? @tqchen

tqchen · 2020-03-26T16:03:12Z

OK, however, note that havinbg a arena based allocator will likely mean remove the vmalloc based versions(for simplicity reasons). Will let @tmoreau89 to manage this PR :)

liangfu · 2020-03-27T00:37:26Z

That make sense, since we can have strategies for both runtime and workspace memories.

tqchen · 2020-03-30T21:01:04Z

@tmoreau89 please followup @liangfu please rebase against the master

tqchen · 2020-04-01T21:09:11Z

ping @liangfu

liangfu · 2020-04-02T01:43:49Z

Rebased. Sorry for the late update.

tqchen

one last minor comment, I think we can then merge it and move on to the arena based allocator

include/tvm/runtime/crt/logging.h

tmoreau89

Thanks Liangfu for these changes! One more question I have before approving the changes. Are the C sources currently being linted?

liangfu · 2020-04-03T04:21:25Z

Are the C sources currently being linted?

Yes, all sources (including C and C++ source code) under src directory are checked by the cpplint, see https://github.com/apache/incubator-tvm/blob/6e1cd8256e461deca51e45e7aae33088e8f5b966/Makefile#L76

Change-Id: I0f79f909a04d1c677aabb80f202f0612c5ce7f2a

Change-Id: I37104c09e28112b1974fa2b064c809d0a8d686c3

Change-Id: I039b12015a1d56c8f4120867cd5a5292da34f3e3

Change-Id: I35800470bcbfcf96652494f359711cb4c2d34398

Change-Id: I72071289843fff4031c0df8796868a0b9fbc57ee

Change-Id: I32dba85ac1660c77f51c2d0d8ab6436ed0c01c74

Change-Id: If12cd240685d7791fc60bc0cfb66389cdc186b73

Change-Id: I7c9d90c11b60b8edda2427ebd189ebe535af2100

Change-Id: I939fa43027a5c7529c5c7c6bd8d6e6beb91b7581

Change-Id: Ie4ff6b64fdfe6810836cf8fd44dace82a20c4581

Change-Id: Ibf3c06619ef296df5c49f3945cb6428777781d69

tqchen · 2020-04-06T16:22:48Z

LGTM from my side, @liangfu let us also make sure we add the crt test to the CI. possibly by running some of the quick tests in the apps

tmoreau89

Thanks @liangfu the changes LGTM

tmoreau89 · 2020-04-07T21:33:39Z

Thanks @liangfu @tqchen the PR has been merged. For the CRT CI tests, I suggest these get added in a follow up PR.

* initial crt_memory and memory leak fix in graph_runtime Change-Id: I0f79f909a04d1c677aabb80f202f0612c5ce7f2a * fix memory leak Change-Id: I37104c09e28112b1974fa2b064c809d0a8d686c3 * clean up Change-Id: I039b12015a1d56c8f4120867cd5a5292da34f3e3 * implement vrealloc Change-Id: I35800470bcbfcf96652494f359711cb4c2d34398 * allocate from stack memory for most of the variables Change-Id: I72071289843fff4031c0df8796868a0b9fbc57ee * allocate from stack memory for all of the variables Change-Id: I32dba85ac1660c77f51c2d0d8ab6436ed0c01c74 * lint Change-Id: If12cd240685d7791fc60bc0cfb66389cdc186b73 * lint Change-Id: I7c9d90c11b60b8edda2427ebd189ebe535af2100 * facilitate the growth of TVM_CRT_MAX_NDIM Change-Id: I939fa43027a5c7529c5c7c6bd8d6e6beb91b7581 * extend test coverage of vmalloc Change-Id: Ie4ff6b64fdfe6810836cf8fd44dace82a20c4581 * lint Change-Id: Ibf3c06619ef296df5c49f3945cb6428777781d69 * move logging.h to src * fix an error in macOS * remove logging.h * use cflags for gcc * fix compilation error

liangfu mentioned this pull request Mar 22, 2020

[uTVM][Runtime] Deprecate uTVM Standalone Runtime #5060

Closed

9 tasks

liangfu marked this pull request as ready for review March 23, 2020 06:54

tqchen added the status: need review label Mar 23, 2020

tqchen assigned tmoreau89 Mar 23, 2020

tqchen self-assigned this Mar 23, 2020

tmoreau89 reviewed Mar 23, 2020

View reviewed changes

tqchen added the status: need update need update based on feedbacks label Mar 30, 2020

liangfu force-pushed the crt_memory branch from 24b26a1 to 7914ba2 Compare April 2, 2020 01:38

tqchen requested changes Apr 2, 2020

View reviewed changes

include/tvm/runtime/crt/logging.h Outdated Show resolved Hide resolved

tmoreau89 reviewed Apr 2, 2020

View reviewed changes

liangfu added 8 commits April 3, 2020 18:03

initial crt_memory and memory leak fix in graph_runtime

f377d68

Change-Id: I0f79f909a04d1c677aabb80f202f0612c5ce7f2a

fix memory leak

4f12f54

Change-Id: I37104c09e28112b1974fa2b064c809d0a8d686c3

clean up

e2ff2e4

Change-Id: I039b12015a1d56c8f4120867cd5a5292da34f3e3

implement vrealloc

9cd4e13

Change-Id: I35800470bcbfcf96652494f359711cb4c2d34398

allocate from stack memory for most of the variables

4707191

Change-Id: I72071289843fff4031c0df8796868a0b9fbc57ee

allocate from stack memory for all of the variables

c8fc368

Change-Id: I32dba85ac1660c77f51c2d0d8ab6436ed0c01c74

lint

e708e6f

Change-Id: If12cd240685d7791fc60bc0cfb66389cdc186b73

lint

c1a118a

Change-Id: I7c9d90c11b60b8edda2427ebd189ebe535af2100

liangfu and others added 4 commits April 3, 2020 18:03

facilitate the growth of TVM_CRT_MAX_NDIM

6c0d012

Change-Id: I939fa43027a5c7529c5c7c6bd8d6e6beb91b7581

extend test coverage of vmalloc

364e9e5

Change-Id: Ie4ff6b64fdfe6810836cf8fd44dace82a20c4581

lint

24bf74d

Change-Id: Ibf3c06619ef296df5c49f3945cb6428777781d69

move logging.h to src

30a50f6

liangfu force-pushed the crt_memory branch from 7914ba2 to 30a50f6 Compare April 3, 2020 10:59

liangfu added 4 commits April 3, 2020 19:11

fix an error in macOS

2eda3f7

remove logging.h

799df93

use cflags for gcc

837b233

fix compilation error

1bcd212

liangfu requested review from tmoreau89 and tqchen April 6, 2020 14:27

tqchen approved these changes Apr 6, 2020

View reviewed changes

tmoreau89 reviewed Apr 7, 2020

View reviewed changes

tmoreau89 merged commit e11a609 into apache:master Apr 7, 2020

tqchen added status: accepted and removed status: need review status: need update need update based on feedbacks labels Apr 7, 2020

ZihengJiang mentioned this pull request Sep 25, 2020

TVM v0.7 Release Note Candidate #6486

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[uTVM][Runtime] Introduce Virtual Memory Allocator to CRT #5124

[uTVM][Runtime] Introduce Virtual Memory Allocator to CRT #5124

liangfu commented Mar 22, 2020

liangfu commented Mar 23, 2020

tqchen commented Mar 23, 2020

tmoreau89 left a comment

tmoreau89 Mar 23, 2020

liangfu Mar 26, 2020

tmoreau89 Mar 23, 2020

liangfu Mar 26, 2020

liangfu Mar 26, 2020

tmoreau89 Mar 23, 2020

liangfu Mar 26, 2020

tqchen commented Mar 24, 2020

liangfu commented Mar 24, 2020

tqchen commented Mar 24, 2020 •

edited

Loading

liangfu commented Mar 26, 2020

tqchen commented Mar 26, 2020 •

edited

Loading

liangfu commented Mar 27, 2020

tqchen commented Mar 30, 2020

tqchen commented Apr 1, 2020

liangfu commented Apr 2, 2020

tqchen left a comment

tmoreau89 left a comment

liangfu commented Apr 3, 2020 •

edited

Loading

tqchen commented Apr 6, 2020

tmoreau89 left a comment

tmoreau89 commented Apr 7, 2020

[uTVM][Runtime] Introduce Virtual Memory Allocator to CRT #5124

[uTVM][Runtime] Introduce Virtual Memory Allocator to CRT #5124

Conversation

liangfu commented Mar 22, 2020

liangfu commented Mar 23, 2020

tqchen commented Mar 23, 2020

tmoreau89 left a comment

Choose a reason for hiding this comment

tmoreau89 Mar 23, 2020

Choose a reason for hiding this comment

liangfu Mar 26, 2020

Choose a reason for hiding this comment

tmoreau89 Mar 23, 2020

Choose a reason for hiding this comment

liangfu Mar 26, 2020

Choose a reason for hiding this comment

liangfu Mar 26, 2020

Choose a reason for hiding this comment

tmoreau89 Mar 23, 2020

Choose a reason for hiding this comment

liangfu Mar 26, 2020

Choose a reason for hiding this comment

tqchen commented Mar 24, 2020

liangfu commented Mar 24, 2020

tqchen commented Mar 24, 2020 • edited Loading

liangfu commented Mar 26, 2020

tqchen commented Mar 26, 2020 • edited Loading

liangfu commented Mar 27, 2020

tqchen commented Mar 30, 2020

tqchen commented Apr 1, 2020

liangfu commented Apr 2, 2020

tqchen left a comment

Choose a reason for hiding this comment

tmoreau89 left a comment

Choose a reason for hiding this comment

liangfu commented Apr 3, 2020 • edited Loading

tqchen commented Apr 6, 2020

tmoreau89 left a comment

Choose a reason for hiding this comment

tmoreau89 commented Apr 7, 2020

tqchen commented Mar 24, 2020 •

edited

Loading

tqchen commented Mar 26, 2020 •

edited

Loading

liangfu commented Apr 3, 2020 •

edited

Loading