[µTVM] Use standalone_crt build tree for all µTVM builds #7333

areusch · 2021-01-23T02:47:58Z

Right now due to historical reasons we sometimes build the CRT from build/standalone_crt and other times from src/runtime/crt. This PR consolidates building to always be from build/standalone_crt and updates several path computations to use that. Ultimately we'll need this if we expect microTVM to be useful when TVM has been pip installed rather than just built from source.

This is also helpful for autotuning, as we expect the autotvm runner to use standalone_crt to build the project. This PR also changes the default compiler options to reference the standalone_crt directory.

Finally, to properly separate the host main() and crt_config.h from the remaining reusable CRT libs, places these in a new directory standalone_crt/template along with the template crt_config.h.

…tree

…ne_crt

manupak · 2021-01-29T07:35:51Z

Hi @areusch,

This is great!
Out of curiosity, the basic purpose of the PR is essentially move the crt sources to build directory right ? -- so that they get shipped with the wheel ?

areusch · 2021-01-29T17:32:39Z

@manupa-arm yeah correct--it revises all functions in tvm.micro to operate relative to either a default standalone_crt directory, which now resides in build, or to one you specify. for autotuning, this becomes more helpful as i've moved compilation to the runner, so the standalone_crt directory will be different than the default. but, it also will help in future cases where standalone_crt is included as a wheel data file and it is necessary to write it out to disk in a project-specific location.

* Make build/standalone_crt available to CI unit tests. * We could do this with stash but it's complex because we would need to use wildcards. Instead, rebuild standalone_crt source tree before testing.

…tree

areusch · 2021-02-09T05:57:01Z

@tqchen @manupa-arm @u99127 @gromero @tom-gall looks like this is ready for review

gromero · 2021-02-09T22:44:36Z

@areusch Hi Andrew. The change looks good (I'll differ the review of Jenkinsfile to others). I just suggest to fit docstrings of get_standalone_crt_dir and get_standalone_crt_lib() in 80 columns maximum. Also, a side note on the PR itself, if possible, I'd avoid merges in the middle of the incremental changes pushed to the branch to be reviewed, because it makes a tad difficult to squash the commits into the final change, in that sense I think rebasing on top of master is better. I tested micro_tflite.py against a disco board too and it's good. So, thumbs up from my side.

areusch · 2021-02-10T00:33:28Z

@gromero thanks for the review. unfortunately if you force-push (I.e. rebase) it loses the context around the previous comments, and I like that less :/. for line length, we're at 100 char, and pylint's enforcing that, so i'm keeping to that here.

manupak

Broadly looks fine. few clarifications and concerns.

manupak · 2021-02-10T10:02:56Z

python/tvm/micro/build.py

@@ -55,15 +57,62 @@ def path(self):
 CRT_RUNTIME_LIB_NAMES = ["utvm_rpc_server", "utvm_rpc_common", "common"]


-TVM_ROOT_DIR = os.path.realpath(os.path.join(os.path.dirname(__file__), "..", "..", ".."))
+STANDALONE_CRT_DIR = None


Im not too sure whether we need this ?

do you mean that it's fine to just do the search every time?

oh you want to cache/memoize the function ... if thats the case, you might want to consider options to do that such using the memoize decorator, functools caches or maybe we can move variable inside of the function as it is not used by anyother ?

we can, though functools.cache is new in 3.9 and lru_cache seems a bit complex. i think this way might almost be easier to debug. is there a different stdlib thing i'm missing?

Hmm there is one other option you might want to consider -- you make it attr of the function -- thats python's way of having static variables. Anyway, Im not very fussed about it.

ok--I may opt to leave this as it's passing CI and there's a bunch of other pressing work.

manupak · 2021-02-10T10:04:18Z

python/tvm/micro/build.py

+        The path to the standalone_crt
+    """
+    global STANDALONE_CRT_DIR
+    if STANDALONE_CRT_DIR is None:


Again on the same question as above, however this kind of makes me think do we need something like an environment variable ? Maybe PassContext is a better option for that ? What do you think ?

I think that ultimately these are going to become data dependencies of the tlcpack wheel and we don't have a standard facility to figure out where the data deps go. I think we should make such a thing in both Python and CMake and then it would reduce this function to: if not isdir(f'{data_dir}/standalone_crt'): raise CrtNotFoundError().

manupak · 2021-02-10T10:08:37Z

python/tvm/micro/build.py

-        f"{TVM_ROOT_DIR}/include",
-        f"{TVM_ROOT_DIR}/3rdparty/dlpack/include",
-        f"{TVM_ROOT_DIR}/3rdparty/libcrc/include",
-        f"{TVM_ROOT_DIR}/3rdparty/dmlc-core/include",


[Clarification] do we not need them anymore or do they go inside build/crt/include ?

they go in build/standalone_crt/include according to cmake/modules/StandaloneCrt.cmake.

manupak · 2021-02-10T12:58:45Z

python/tvm/micro/build.py

-_CRT_GENERATED_LIB_OPTIONS["cflags"].append("-Wno-unused-variable")
-_CRT_GENERATED_LIB_OPTIONS["ccflags"].append("-Wno-unused-variable")
+    lib_opts = _build_default_compiler_options(standalone_crt_dir)
+    lib_opts["cflags"] = ["-Wno-error=incompatible-pointer-types"]


[Clarification] why is this needed suddenly ?

I think it's on line 110 at main--I don't think it's a change here?

Oh sorry I missed that!

gromero · 2021-02-10T15:23:56Z

@gromero thanks for the review. unfortunately if you force-push (I.e. rebase) it loses the context around the previous comments, and I like that less :/.

@areusch yeah.. I forgot about the "constraint" on push -f. Maybe the only thing we could do is to try to avoid merging in the middle of the incremental pushes (pushes due to the review process)? On the other hand that amounts to not doing a rebase on the top of the most fresh code in main, but I wonder if github and the CI is actually already covering what might go wrong if rebase is not done (github will check for merge conflicts on top of the main branch, but I'm not sure about the CI, I guess it only builds and tests the branch ones want to merge into main, i.e. it doesn't build the commits found in github's "Commits" tab on top of the main branch). Yep, tricky thing, merging in the middle of the incremental pushes for complicated changes might get really hard to review. I see.

areusch · 2021-02-10T18:17:35Z

@gromero easiest way to patch given merges might be something like:

$ git fetch origin main
$ git fetch areusch <pr>
$ git diff areusch/<pr>...origin/main >patch # NOTE 3 periods
$ git checkout $(merge-base areusch/<pr> origin/main)
$ git apply <patch

I think the rest is just limitations of this code-review system. I generally try to push -f until there's a comment on the thread

areusch

@manupa-arm thanks for the review!

areusch · 2021-02-10T19:17:34Z

python/tvm/micro/build.py

@@ -55,15 +57,62 @@ def path(self):
 CRT_RUNTIME_LIB_NAMES = ["utvm_rpc_server", "utvm_rpc_common", "common"]


-TVM_ROOT_DIR = os.path.realpath(os.path.join(os.path.dirname(__file__), "..", "..", ".."))
+STANDALONE_CRT_DIR = None


do you mean that it's fine to just do the search every time?

areusch · 2021-02-10T19:18:58Z

python/tvm/micro/build.py

+        The path to the standalone_crt
+    """
+    global STANDALONE_CRT_DIR
+    if STANDALONE_CRT_DIR is None:


I think that ultimately these are going to become data dependencies of the tlcpack wheel and we don't have a standard facility to figure out where the data deps go. I think we should make such a thing in both Python and CMake and then it would reduce this function to: if not isdir(f'{data_dir}/standalone_crt'): raise CrtNotFoundError().

areusch · 2021-02-10T19:19:23Z

python/tvm/micro/build.py

-        f"{TVM_ROOT_DIR}/include",
-        f"{TVM_ROOT_DIR}/3rdparty/dlpack/include",
-        f"{TVM_ROOT_DIR}/3rdparty/libcrc/include",
-        f"{TVM_ROOT_DIR}/3rdparty/dmlc-core/include",


they go in build/standalone_crt/include according to cmake/modules/StandaloneCrt.cmake.

areusch · 2021-02-10T19:21:04Z

python/tvm/micro/build.py

-_CRT_GENERATED_LIB_OPTIONS["cflags"].append("-Wno-unused-variable")
-_CRT_GENERATED_LIB_OPTIONS["ccflags"].append("-Wno-unused-variable")
+    lib_opts = _build_default_compiler_options(standalone_crt_dir)
+    lib_opts["cflags"] = ["-Wno-error=incompatible-pointer-types"]


I think it's on line 110 at main--I don't think it's a change here?

gromero · 2021-02-10T20:30:20Z

@gromero easiest way to patch given merges might be something like:

$ git fetch origin main
$ git fetch areusch <pr>
$ git diff areusch/<pr>...origin/main >patch # NOTE 3 periods
$ git checkout $(merge-base areusch/<pr> origin/main)
$ git apply <patch

@areusch I see. I imagine that's the flow the committer will follow when squashing for the final commit to get merged into main? Because if one does a git rebase -i straight to the branch related to the PR with the incremental changes "intertwined" with the commits brought by the merges it would kind of a nightmare to squash afaics?

Now, I've just made a quick test and I see that the review comments (even the comments related to a line of code) haven't disappeared after a git push -f, so it does really keep the review history. What I did on the experiment for a PR under review was simply the following, whilst checked out in the PR branch (pr) and with an incremental change already committed to the branch:

git pull --rebase origin main
git push -f <my_repo> pr

The forced push and the new incremental change (plus the changes merged / pushed into main meanwhile) were all correctly displayed below the review comments posted previously to it.

Feel free to ping for a few tests if you want :)

I generally try to push -f until there's a comment on the thread

Yeah, that's what I do too.

areusch · 2021-02-10T20:57:20Z

@gromero yes, I think that's roughly what GH does when someone hits merge.

the review comments do stick around, but the context is usually obliterated, which is usually pretty crucial to understanding what they meant. maybe see if the old sha is still available after a few hours--GH may do a cleanup task internally

gromero · 2021-02-10T21:14:59Z

@gromero yes, I think that's roughly what GH does when someone hits merge.

Got it 👍

the review comments do stick around, but the context is usually obliterated, which is usually pretty crucial to understanding what they meant. maybe see if the old sha is still available after a few hours--GH may do a cleanup task internally

@areusch Amazing, indeed, after some time, when clicking on "View Changes" related to a commit that was rebased (and git pushed -f) I get:

We went looking everywhere, but couldn’t find those commits.
Sometimes commits can disappear after a force-push. Head back to the latest changes here.

The best part is "Sometimes commits can disappear... ". OK, I give up :P

areusch · 2021-02-11T19:17:21Z

@gromero @manupa-arm can you explicit ack if you're ok with this change?

https://tvm.apache.org/docs/contribute/code_review.html#approve-and-request-changes-explicitly

manupak

LGTM

areusch · 2021-02-12T16:22:07Z

@tmoreau89 @tqchen to merge

tmoreau89

LGTM

tmoreau89 · 2021-02-12T23:42:08Z

Thank you @areusch @gromero @manupa-arm - the PR has been merged

* Build microTVM using standalone_crt in build tree. * black format * pylint * try stashing entire standalone_crt in hopes it will not upset jenkins * Put standalone_crt in correct Jenkinsfile stash bundle * include build prefix * switch to python script for expanding globs * revert attempt to use globs in pack_libs, switch to building standalone_crt * properly revert pack_lib changes * fix typo * retrigger CI * revert pyproject.toml * update Jenkinsfile approach to use task_ci_setup.sh

areusch added 8 commits January 22, 2021 18:42

Build microTVM using standalone_crt in build tree.

9db961d

black format

4ae8dcb

pylint

0935452

try stashing entire standalone_crt in hopes it will not upset jenkins

03e2edc

Merge remote-tracking branch 'origin/main' into standalone-crt-build-…

0fd91fb

…tree

Put standalone_crt in correct Jenkinsfile stash bundle

fa066e6

include build prefix

0c94604

switch to python script for expanding globs

ae9dcb0

ZihengJiang added the status: need review label Jan 27, 2021

areusch added 2 commits January 28, 2021 10:13

revert attempt to use globs in pack_libs, switch to building standalo…

c0f46b6

…ne_crt

properly revert pack_lib changes

e85c94c

areusch added 2 commits January 29, 2021 09:57

fix typo

d614c0b

retrigger CI

a41c540

tqchen pushed a commit that referenced this pull request Feb 8, 2021

Jenkinsfile changes for #7333. (#7388)

5103bb6

areusch added 3 commits February 8, 2021 12:13

revert pyproject.toml

f7c4f8e

Merge remote-tracking branch 'origin/main' into standalone-crt-build-…

bb26982

…tree

update Jenkinsfile approach to use task_ci_setup.sh

7ea945b

manupak reviewed Feb 10, 2021

View reviewed changes

areusch commented Feb 10, 2021

View reviewed changes

manupak approved these changes Feb 11, 2021

View reviewed changes

alexwong pushed a commit to alexwong/tvm that referenced this pull request Feb 11, 2021

Jenkinsfile changes for apache#7333. (apache#7388)

e64f3a1

gromero approved these changes Feb 12, 2021

View reviewed changes

tmoreau89 approved these changes Feb 12, 2021

View reviewed changes

tmoreau89 merged commit b8a8340 into apache:main Feb 12, 2021

Lokiiiiii pushed a commit to Lokiiiiii/tvm that referenced this pull request Mar 2, 2021

Jenkinsfile changes for apache#7333. (apache#7388)

6d9dd7f

trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Mar 2, 2021

Jenkinsfile changes for apache#7333. (apache#7388)

b7b7e47

junrushao mentioned this pull request Nov 1, 2021

Apache TVM v0.8 Release Note Candidate #9416

Closed

[µTVM] Use standalone_crt build tree for all µTVM builds #7333

[µTVM] Use standalone_crt build tree for all µTVM builds #7333

Conversation

areusch commented Jan 23, 2021

manupak commented Jan 29, 2021

areusch commented Jan 29, 2021

areusch commented Feb 9, 2021

gromero commented Feb 9, 2021 • edited Loading

areusch commented Feb 10, 2021

manupak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gromero commented Feb 10, 2021

areusch commented Feb 10, 2021

areusch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gromero commented Feb 10, 2021 • edited Loading

areusch commented Feb 10, 2021

gromero commented Feb 10, 2021

areusch commented Feb 11, 2021

manupak left a comment

Choose a reason for hiding this comment

areusch commented Feb 12, 2021

tmoreau89 left a comment

Choose a reason for hiding this comment

tmoreau89 commented Feb 12, 2021

gromero commented Feb 9, 2021 •

edited

Loading

gromero commented Feb 10, 2021 •

edited

Loading