You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/01/23 02:48:14 UTC
[GitHub] [tvm] areusch opened a new pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
areusch opened a new pull request #7333:
URL: https://github.com/apache/tvm/pull/7333
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 install`ed 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.
----------------------------------------------------------------
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
[GitHub] [tvm] gromero commented on pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
gromero commented on pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#issuecomment-776294700
@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 also tested `micro_tflite.py` against a disco board and it's all right. So, thumbs up from my side.
----------------------------------------------------------------
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
[GitHub] [tvm] gromero edited a comment on pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
gromero edited a comment on pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#issuecomment-776294700
@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.
----------------------------------------------------------------
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
[GitHub] [tvm] gromero commented on pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
gromero commented on pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#issuecomment-777040897
> @gromero yes, I think that's roughly what GH does when someone hits merge.
Got it :+1:
> 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
----------------------------------------------------------------
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
[GitHub] [tvm] manupa-arm commented on pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#issuecomment-769634956
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 ?
----------------------------------------------------------------
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
[GitHub] [tvm] manupa-arm commented on a change in pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#discussion_r574308382
##########
File path: python/tvm/micro/build.py
##########
@@ -72,52 +121,73 @@ def path(self):
_COMMON_CFLAGS = ["-Wall", "-Werror"]
-_CRT_DEFAULT_OPTIONS = {
- "cflags": ["-std=c11"] + _COMMON_CFLAGS,
- "ccflags": ["-std=c++11"] + _COMMON_CFLAGS,
- "ldflags": ["-std=c++11"],
- "include_dirs": [
- 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",
- f"{CRT_ROOT_DIR}/include",
- ],
-}
+def _build_default_compiler_options(standalone_crt_dir: typing.Optional[str] = None) -> str:
+ """Return a dict containing base compile flags for the CRT under gcc common to .
+ Parameters
+ ----------
+ standalone_crt_dir : Optional[str]
+ If given, the path to the standalone_crt
+ """
+ if standalone_crt_dir is None:
+ standalone_crt_dir = get_standalone_crt_dir()
+ return {
+ "cflags": ["-std=c11"] + _COMMON_CFLAGS,
+ "ccflags": ["-std=c++11"] + _COMMON_CFLAGS,
+ "ldflags": ["-std=c++11"],
+ "include_dirs": [os.path.join(standalone_crt_dir, "include")],
+ }
-_CRT_GENERATED_LIB_OPTIONS = copy.copy(_CRT_DEFAULT_OPTIONS)
+def default_options(crt_config_include_dir, standalone_crt_dir=None):
+ """Return default opts passed to Compile commands.
+
+ Parameters
+ ----------
+ crt_config_include_dir : str
+ Path to a directory containing crt_config.h for the target. This will be appended
+ to the include path for cflags and ccflags.
+ standalone_crt_dir : Optional[str]
+
+ Returns
+ -------
+ Dict :
+ A dictionary containing 3 subkeys, each whose value is _build_default_compiler_options()
+ plus additional customization.
+ - "bin_opts" - passed as "options" to Compiler.binary() when building MicroBinary.
+ - "lib_opts" - passed as "options" to Compiler.library() when building bundled CRT
+ libraries (or otherwise, non-generated libraries).
+ - "generated_lib_opts" - passed as "options" to Compiler.library() when building the
+ generated library.
+ """
+ bin_opts = _build_default_compiler_options(standalone_crt_dir)
+ bin_opts["include_dirs"].append(crt_config_include_dir)
-# Disable due to limitation in the TVM C codegen, which generates lots of local variable
-# declarations at the top of generated code without caring whether they're used.
-# Example:
-# void* arg0 = (((TVMValue*)args)[0].v_handle);
-# int32_t arg0_code = ((int32_t*)arg_type_ids)[(0)];
-_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"]
Review comment:
Oh sorry I missed that!
----------------------------------------------------------------
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
[GitHub] [tvm] manupa-arm commented on a change in pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#discussion_r574308214
##########
File path: python/tvm/micro/build.py
##########
@@ -72,52 +121,73 @@ def path(self):
_COMMON_CFLAGS = ["-Wall", "-Werror"]
-_CRT_DEFAULT_OPTIONS = {
- "cflags": ["-std=c11"] + _COMMON_CFLAGS,
- "ccflags": ["-std=c++11"] + _COMMON_CFLAGS,
- "ldflags": ["-std=c++11"],
- "include_dirs": [
- 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",
Review comment:
Alright!
----------------------------------------------------------------
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
[GitHub] [tvm] areusch commented on pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#issuecomment-777726454
@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
----------------------------------------------------------------
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
[GitHub] [tvm] areusch commented on a change in pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#discussion_r574761585
##########
File path: 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
Review comment:
ok--I may opt to leave this as it's passing CI and there's a bunch of other pressing work.
----------------------------------------------------------------
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
[GitHub] [tvm] areusch commented on pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#issuecomment-769943540
@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.
----------------------------------------------------------------
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
[GitHub] [tvm] areusch commented on pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#issuecomment-778293624
@tmoreau89 @tqchen to merge
----------------------------------------------------------------
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
[GitHub] [tvm] tmoreau89 merged pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
tmoreau89 merged pull request #7333:
URL: https://github.com/apache/tvm/pull/7333
----------------------------------------------------------------
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
[GitHub] [tvm] areusch commented on pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#issuecomment-776343305
@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.
----------------------------------------------------------------
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
[GitHub] [tvm] gromero commented on pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
gromero commented on pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#issuecomment-777008938
> @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 origin 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.
----------------------------------------------------------------
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
[GitHub] [tvm] manupa-arm commented on a change in pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#discussion_r573597149
##########
File path: 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
-CRT_ROOT_DIR = os.path.join(TVM_ROOT_DIR, "src", "runtime", "crt")
+class CrtNotFoundError(Exception):
+ """Raised when the standalone CRT dirtree cannot be found."""
-RUNTIME_LIB_SRC_DIRS = [os.path.join(CRT_ROOT_DIR, n) for n in CRT_RUNTIME_LIB_NAMES] + [
- os.path.join(TVM_ROOT_DIR, "3rdparty/libcrc/src")
-]
+def get_standalone_crt_dir() -> str:
+ """Find the standalone_crt directory.
+
+ Though the C runtime source lives in the tvm tree, it is intended to be distributed with any
+ binary build of TVM. This source tree is intended to be integrated into user projects to run
+ models targeted with --runtime=c.
+
+ Returns
+ -------
+ str :
+ The path to the standalone_crt
+ """
+ global STANDALONE_CRT_DIR
+ if STANDALONE_CRT_DIR is None:
Review comment:
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 ?
##########
File path: 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
Review comment:
Im not too sure whether we need this ?
##########
File path: python/tvm/micro/build.py
##########
@@ -72,52 +121,73 @@ def path(self):
_COMMON_CFLAGS = ["-Wall", "-Werror"]
-_CRT_DEFAULT_OPTIONS = {
- "cflags": ["-std=c11"] + _COMMON_CFLAGS,
- "ccflags": ["-std=c++11"] + _COMMON_CFLAGS,
- "ldflags": ["-std=c++11"],
- "include_dirs": [
- 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",
- f"{CRT_ROOT_DIR}/include",
- ],
-}
+def _build_default_compiler_options(standalone_crt_dir: typing.Optional[str] = None) -> str:
+ """Return a dict containing base compile flags for the CRT under gcc common to .
+ Parameters
+ ----------
+ standalone_crt_dir : Optional[str]
+ If given, the path to the standalone_crt
+ """
+ if standalone_crt_dir is None:
+ standalone_crt_dir = get_standalone_crt_dir()
+ return {
+ "cflags": ["-std=c11"] + _COMMON_CFLAGS,
+ "ccflags": ["-std=c++11"] + _COMMON_CFLAGS,
+ "ldflags": ["-std=c++11"],
+ "include_dirs": [os.path.join(standalone_crt_dir, "include")],
+ }
-_CRT_GENERATED_LIB_OPTIONS = copy.copy(_CRT_DEFAULT_OPTIONS)
+def default_options(crt_config_include_dir, standalone_crt_dir=None):
+ """Return default opts passed to Compile commands.
+
+ Parameters
+ ----------
+ crt_config_include_dir : str
+ Path to a directory containing crt_config.h for the target. This will be appended
+ to the include path for cflags and ccflags.
+ standalone_crt_dir : Optional[str]
+
+ Returns
+ -------
+ Dict :
+ A dictionary containing 3 subkeys, each whose value is _build_default_compiler_options()
+ plus additional customization.
+ - "bin_opts" - passed as "options" to Compiler.binary() when building MicroBinary.
+ - "lib_opts" - passed as "options" to Compiler.library() when building bundled CRT
+ libraries (or otherwise, non-generated libraries).
+ - "generated_lib_opts" - passed as "options" to Compiler.library() when building the
+ generated library.
+ """
+ bin_opts = _build_default_compiler_options(standalone_crt_dir)
+ bin_opts["include_dirs"].append(crt_config_include_dir)
-# Disable due to limitation in the TVM C codegen, which generates lots of local variable
-# declarations at the top of generated code without caring whether they're used.
-# Example:
-# void* arg0 = (((TVMValue*)args)[0].v_handle);
-# int32_t arg0_code = ((int32_t*)arg_type_ids)[(0)];
-_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"]
Review comment:
[Clarification] why is this needed suddenly ?
##########
File path: python/tvm/micro/build.py
##########
@@ -72,52 +121,73 @@ def path(self):
_COMMON_CFLAGS = ["-Wall", "-Werror"]
-_CRT_DEFAULT_OPTIONS = {
- "cflags": ["-std=c11"] + _COMMON_CFLAGS,
- "ccflags": ["-std=c++11"] + _COMMON_CFLAGS,
- "ldflags": ["-std=c++11"],
- "include_dirs": [
- 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",
Review comment:
[Clarification] do we not need them anymore or do they go inside build/crt/include ?
----------------------------------------------------------------
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
[GitHub] [tvm] areusch commented on pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#issuecomment-775689400
@tqchen @manupa-arm @u99127 @gromero @tom-gall looks like this is ready for review
----------------------------------------------------------------
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
[GitHub] [tvm] areusch commented on a change in pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#discussion_r574754851
##########
File path: 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
Review comment:
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?
----------------------------------------------------------------
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
[GitHub] [tvm] areusch commented on pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#issuecomment-777030634
@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
----------------------------------------------------------------
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
[GitHub] [tvm] gromero edited a comment on pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
gromero edited a comment on pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#issuecomment-777008938
> @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.
----------------------------------------------------------------
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
[GitHub] [tvm] areusch commented on a change in pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#discussion_r574006510
##########
File path: 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
Review comment:
do you mean that it's fine to just do the search every time?
##########
File path: python/tvm/micro/build.py
##########
@@ -72,52 +121,73 @@ def path(self):
_COMMON_CFLAGS = ["-Wall", "-Werror"]
-_CRT_DEFAULT_OPTIONS = {
- "cflags": ["-std=c11"] + _COMMON_CFLAGS,
- "ccflags": ["-std=c++11"] + _COMMON_CFLAGS,
- "ldflags": ["-std=c++11"],
- "include_dirs": [
- 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",
Review comment:
they go in build/standalone_crt/include according to cmake/modules/StandaloneCrt.cmake.
##########
File path: 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
-CRT_ROOT_DIR = os.path.join(TVM_ROOT_DIR, "src", "runtime", "crt")
+class CrtNotFoundError(Exception):
+ """Raised when the standalone CRT dirtree cannot be found."""
-RUNTIME_LIB_SRC_DIRS = [os.path.join(CRT_ROOT_DIR, n) for n in CRT_RUNTIME_LIB_NAMES] + [
- os.path.join(TVM_ROOT_DIR, "3rdparty/libcrc/src")
-]
+def get_standalone_crt_dir() -> str:
+ """Find the standalone_crt directory.
+
+ Though the C runtime source lives in the tvm tree, it is intended to be distributed with any
+ binary build of TVM. This source tree is intended to be integrated into user projects to run
+ models targeted with --runtime=c.
+
+ Returns
+ -------
+ str :
+ The path to the standalone_crt
+ """
+ global STANDALONE_CRT_DIR
+ if STANDALONE_CRT_DIR is None:
Review comment:
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()`.
##########
File path: python/tvm/micro/build.py
##########
@@ -72,52 +121,73 @@ def path(self):
_COMMON_CFLAGS = ["-Wall", "-Werror"]
-_CRT_DEFAULT_OPTIONS = {
- "cflags": ["-std=c11"] + _COMMON_CFLAGS,
- "ccflags": ["-std=c++11"] + _COMMON_CFLAGS,
- "ldflags": ["-std=c++11"],
- "include_dirs": [
- 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",
- f"{CRT_ROOT_DIR}/include",
- ],
-}
+def _build_default_compiler_options(standalone_crt_dir: typing.Optional[str] = None) -> str:
+ """Return a dict containing base compile flags for the CRT under gcc common to .
+ Parameters
+ ----------
+ standalone_crt_dir : Optional[str]
+ If given, the path to the standalone_crt
+ """
+ if standalone_crt_dir is None:
+ standalone_crt_dir = get_standalone_crt_dir()
+ return {
+ "cflags": ["-std=c11"] + _COMMON_CFLAGS,
+ "ccflags": ["-std=c++11"] + _COMMON_CFLAGS,
+ "ldflags": ["-std=c++11"],
+ "include_dirs": [os.path.join(standalone_crt_dir, "include")],
+ }
-_CRT_GENERATED_LIB_OPTIONS = copy.copy(_CRT_DEFAULT_OPTIONS)
+def default_options(crt_config_include_dir, standalone_crt_dir=None):
+ """Return default opts passed to Compile commands.
+
+ Parameters
+ ----------
+ crt_config_include_dir : str
+ Path to a directory containing crt_config.h for the target. This will be appended
+ to the include path for cflags and ccflags.
+ standalone_crt_dir : Optional[str]
+
+ Returns
+ -------
+ Dict :
+ A dictionary containing 3 subkeys, each whose value is _build_default_compiler_options()
+ plus additional customization.
+ - "bin_opts" - passed as "options" to Compiler.binary() when building MicroBinary.
+ - "lib_opts" - passed as "options" to Compiler.library() when building bundled CRT
+ libraries (or otherwise, non-generated libraries).
+ - "generated_lib_opts" - passed as "options" to Compiler.library() when building the
+ generated library.
+ """
+ bin_opts = _build_default_compiler_options(standalone_crt_dir)
+ bin_opts["include_dirs"].append(crt_config_include_dir)
-# Disable due to limitation in the TVM C codegen, which generates lots of local variable
-# declarations at the top of generated code without caring whether they're used.
-# Example:
-# void* arg0 = (((TVMValue*)args)[0].v_handle);
-# int32_t arg0_code = ((int32_t*)arg_type_ids)[(0)];
-_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"]
Review comment:
I think it's on line 110 at main--I don't think it's a change 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:
users@infra.apache.org
[GitHub] [tvm] areusch commented on pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#issuecomment-776911131
@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
----------------------------------------------------------------
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
[GitHub] [tvm] manupa-arm commented on a change in pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#discussion_r574307762
##########
File path: 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
Review comment:
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 ?
----------------------------------------------------------------
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
[GitHub] [tvm] tmoreau89 commented on pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#issuecomment-778515054
Thank you @areusch @gromero @manupa-arm - the PR has been merged
----------------------------------------------------------------
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
[GitHub] [tvm] gromero commented on pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
gromero commented on pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#issuecomment-776787476
> @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.
----------------------------------------------------------------
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
[GitHub] [tvm] manupa-arm commented on a change in pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#discussion_r574693544
##########
File path: 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
-CRT_ROOT_DIR = os.path.join(TVM_ROOT_DIR, "src", "runtime", "crt")
+class CrtNotFoundError(Exception):
+ """Raised when the standalone CRT dirtree cannot be found."""
-RUNTIME_LIB_SRC_DIRS = [os.path.join(CRT_ROOT_DIR, n) for n in CRT_RUNTIME_LIB_NAMES] + [
- os.path.join(TVM_ROOT_DIR, "3rdparty/libcrc/src")
-]
+def get_standalone_crt_dir() -> str:
+ """Find the standalone_crt directory.
+
+ Though the C runtime source lives in the tvm tree, it is intended to be distributed with any
+ binary build of TVM. This source tree is intended to be integrated into user projects to run
+ models targeted with --runtime=c.
+
+ Returns
+ -------
+ str :
+ The path to the standalone_crt
+ """
+ global STANDALONE_CRT_DIR
+ if STANDALONE_CRT_DIR is None:
Review comment:
Ack
----------------------------------------------------------------
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
[GitHub] [tvm] manupa-arm commented on a change in pull request #7333: [µTVM] Use standalone_crt build tree for all µTVM builds
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7333:
URL: https://github.com/apache/tvm/pull/7333#discussion_r574760571
##########
File path: 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
Review comment:
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.
----------------------------------------------------------------
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