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 2022/11/07 19:11:35 UTC
[GitHub] [tvm] mehrdadh opened a new pull request, #13313: [microTVM][CRT]Add memory size as project option
mehrdadh opened a new pull request, #13313:
URL: https://github.com/apache/tvm/pull/13313
This PR changes memory size to a project option so user can set it at compile time.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a diff in pull request #13313: [microTVM][CRT] Add memory size as project option
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #13313:
URL: https://github.com/apache/tvm/pull/13313#discussion_r1016873781
##########
src/runtime/crt/host/microtvm_api_server.py:
##########
@@ -67,6 +80,27 @@ def server_info_query(self, tvm_version):
# The build target given to make
BUILD_TARGET = "build/main"
+ def _populate_makefile(
+ self,
+ makefile_template_path: pathlib.Path,
+ makefile_path: pathlib.Path,
+ memory_size: int,
+ ):
+ """Generate Makefile from template."""
+ flags = {
+ "MEMORY_SIZE_BYTES": str(memory_size),
+ }
+
+ regex = re.compile(r"([A-Z_]+) := (<[A-Z_]+>)")
+ with open(makefile_path, "w") as makefile_f:
+ with open(makefile_template_path, "r") as makefile_template_f:
+ for line in makefile_template_f:
+ m = regex.match(line)
+ if m:
+ var, token = m.groups()
+ line = re.sub(token, flags[var], line)
Review Comment:
done
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a diff in pull request #13313: [microTVM][CRT] Add memory size as project option
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #13313:
URL: https://github.com/apache/tvm/pull/13313#discussion_r1016038666
##########
src/runtime/crt/host/microtvm_api_server.py:
##########
@@ -57,7 +71,14 @@ def server_info_query(self, tvm_version):
optional=["build"],
type="bool",
help="Run make with verbose output",
- )
+ ),
+ server.ProjectOption(
+ "memory_size_bytes",
+ optional=["generate_project"],
+ type="int",
+ default=None,
Review Comment:
I kept the default as None, it doesn't matter because if you don't pass the option then the option doesn't exist. So default value doesn't mean anything 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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on pull request #13313: [microTVM][CRT] Add memory size as project option
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on PR #13313:
URL: https://github.com/apache/tvm/pull/13313#issuecomment-1307507801
@gromero PTAL
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on a diff in pull request #13313: [microTVM][CRT] Add memory size as project option
Posted by GitBox <gi...@apache.org>.
gromero commented on code in PR #13313:
URL: https://github.com/apache/tvm/pull/13313#discussion_r1016625167
##########
src/runtime/crt/host/microtvm_api_server.py:
##########
@@ -67,6 +88,29 @@ def server_info_query(self, tvm_version):
# The build target given to make
BUILD_TARGET = "build/main"
+ def _populate_makefile(
+ self,
+ makefile_template_path: pathlib.Path,
+ makefile_path: pathlib.Path,
+ memory_size: int,
Review Comment:
ok
##########
src/runtime/crt/host/microtvm_api_server.py:
##########
@@ -57,7 +71,14 @@ def server_info_query(self, tvm_version):
optional=["build"],
type="bool",
help="Run make with verbose output",
- )
+ ),
+ server.ProjectOption(
+ "memory_size_bytes",
+ optional=["generate_project"],
+ type="int",
+ default=None,
Review Comment:
If the option doesn't exist _there is a default value_, because `options.get("memory_size_bytes", MEMORY_SIZE_BYTES)` is passed to `_populate_makefile` and so `MEMORY_SIZE_BYTES` will be used as the default value in the server, no?
The `default` metadatum is there not for the server, but mostly for the client, so if there is a default value that is used by the server if the option is not present, then it must be informed, as per:
https://github.com/apache/tvm-rfcs/blob/main/rfcs/0020-project_api_extend_metadata.md#on-default-metadatum
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on a diff in pull request #13313: [microTVM][CRT] Add memory size as project option
Posted by GitBox <gi...@apache.org>.
gromero commented on code in PR #13313:
URL: https://github.com/apache/tvm/pull/13313#discussion_r1016894307
##########
src/runtime/crt/host/microtvm_api_server.py:
##########
@@ -57,7 +71,14 @@ def server_info_query(self, tvm_version):
optional=["build"],
type="bool",
help="Run make with verbose output",
- )
+ ),
+ server.ProjectOption(
+ "memory_size_bytes",
+ optional=["generate_project"],
+ type="int",
+ default=None,
Review Comment:
@mehrdadh Maybe we can improve this situation by processing the options based on `PROJECT_OPTIONS` and taking `default` field into account.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on a diff in pull request #13313: [microTVM][CRT] Add memory size as project option
Posted by GitBox <gi...@apache.org>.
gromero commented on code in PR #13313:
URL: https://github.com/apache/tvm/pull/13313#discussion_r1016021735
##########
src/runtime/crt/host/microtvm_api_server.py:
##########
@@ -67,6 +88,29 @@ def server_info_query(self, tvm_version):
# The build target given to make
BUILD_TARGET = "build/main"
+ def _populate_makefile(
+ self,
+ makefile_template_path: pathlib.Path,
+ makefile_path: pathlib.Path,
+ memory_size: int,
+ ):
+ """Generate Makefile from template."""
+ flags = {
+ "MEMORY_SIZE_BYTES": str(memory_size),
+ }
+
+ with open(makefile_path, "w") as makefile_f:
+ with open(makefile_template_path, "r") as makefile_template_f:
+ for line in makefile_template_f:
+ SUBST_TOKEN_RE = re.compile(r"<([A-Z_]+)>")
+ outs = []
+ for i, m in enumerate(re.split(SUBST_TOKEN_RE, line)):
+ if i % 2 == 1:
+ m = flags[m]
+ outs.append(m)
+ line = "".join(outs)
+ makefile_f.write(line)
Review Comment:
Could we make it shorter with (or something alike to):
```
token_val = {
"MEMORY": str(memory_size),
}
regex = re.compile(r'([A-Z_]+) := (<[A-Z_]+>)')
with open(makefile_path, "w") as makefile_f:
with open(makefile_template_path, "r") as makefile_template_f:
for line in makefile_template_f:
m = regex.match(line)
if m:
var, token = m.groups()
line = line.replace(token, token_val[var])
makefile_f.write(line)
```
?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a diff in pull request #13313: [microTVM][CRT] Add memory size as project option
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #13313:
URL: https://github.com/apache/tvm/pull/13313#discussion_r1016038634
##########
src/runtime/crt/host/microtvm_api_server.py:
##########
@@ -67,6 +88,29 @@ def server_info_query(self, tvm_version):
# The build target given to make
BUILD_TARGET = "build/main"
+ def _populate_makefile(
+ self,
+ makefile_template_path: pathlib.Path,
+ makefile_path: pathlib.Path,
+ memory_size: int,
Review Comment:
I'd rather this way because it makes it clear at high level APIs, like generate_project, what are the requirement for each option. If we pass `options` to sub-functions, we need to check multiple levels to determine if each option is required/option for each API call.
##########
src/runtime/crt/host/microtvm_api_server.py:
##########
@@ -57,7 +71,14 @@ def server_info_query(self, tvm_version):
optional=["build"],
type="bool",
help="Run make with verbose output",
- )
+ ),
+ server.ProjectOption(
+ "memory_size_bytes",
+ optional=["generate_project"],
+ type="int",
+ default=None,
Review Comment:
done
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tvm-bot commented on pull request #13313: [microTVM][CRT]Add memory size as project option
Posted by GitBox <gi...@apache.org>.
tvm-bot commented on PR #13313:
URL: https://github.com/apache/tvm/pull/13313#issuecomment-1306067413
<!---bot-comment-->
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @-ing them in a comment.
<!--bot-comment-ccs-start-->
* cc @alanmacd, @gromero <sub>See [#10317](https://github.com/apache/tvm/issues/10317) for details</sub><!--bot-comment-ccs-end-->
<sub>Generated by [tvm-bot](https://github.com/apache/tvm/blob/main/ci/README.md#github-actions)</sub>
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh merged pull request #13313: [microTVM][CRT] Add memory size as project option
Posted by GitBox <gi...@apache.org>.
mehrdadh merged PR #13313:
URL: https://github.com/apache/tvm/pull/13313
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on a diff in pull request #13313: [microTVM][CRT] Add memory size as project option
Posted by GitBox <gi...@apache.org>.
gromero commented on code in PR #13313:
URL: https://github.com/apache/tvm/pull/13313#discussion_r1016021735
##########
src/runtime/crt/host/microtvm_api_server.py:
##########
@@ -67,6 +88,29 @@ def server_info_query(self, tvm_version):
# The build target given to make
BUILD_TARGET = "build/main"
+ def _populate_makefile(
+ self,
+ makefile_template_path: pathlib.Path,
+ makefile_path: pathlib.Path,
+ memory_size: int,
+ ):
+ """Generate Makefile from template."""
+ flags = {
+ "MEMORY_SIZE_BYTES": str(memory_size),
+ }
+
+ with open(makefile_path, "w") as makefile_f:
+ with open(makefile_template_path, "r") as makefile_template_f:
+ for line in makefile_template_f:
+ SUBST_TOKEN_RE = re.compile(r"<([A-Z_]+)>")
+ outs = []
+ for i, m in enumerate(re.split(SUBST_TOKEN_RE, line)):
+ if i % 2 == 1:
+ m = flags[m]
+ outs.append(m)
+ line = "".join(outs)
+ makefile_f.write(line)
Review Comment:
Could we make it shorter with (or something alike):
```
token_val = {
"MEMORY": str(memory_size),
}
regex = re.compile(r'([A-Z_]+) := (<[A-Z_]+>)')
with open(makefile_path, "w") as makefile_f:
with open(makefile_template_path, "r") as makefile_template_f:
for line in makefile_template_f:
m = regex.match(line)
if m:
var, token = m.groups()
line = line.replace(token, token_val[var])
makefile_f.write(line)
```
?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on a diff in pull request #13313: [microTVM][CRT] Add memory size as project option
Posted by GitBox <gi...@apache.org>.
gromero commented on code in PR #13313:
URL: https://github.com/apache/tvm/pull/13313#discussion_r1016654014
##########
src/runtime/crt/host/microtvm_api_server.py:
##########
@@ -67,6 +80,27 @@ def server_info_query(self, tvm_version):
# The build target given to make
BUILD_TARGET = "build/main"
+ def _populate_makefile(
+ self,
+ makefile_template_path: pathlib.Path,
+ makefile_path: pathlib.Path,
+ memory_size: int,
+ ):
+ """Generate Makefile from template."""
+ flags = {
+ "MEMORY_SIZE_BYTES": str(memory_size),
+ }
+
+ regex = re.compile(r"([A-Z_]+) := (<[A-Z_]+>)")
+ with open(makefile_path, "w") as makefile_f:
+ with open(makefile_template_path, "r") as makefile_template_f:
+ for line in makefile_template_f:
+ m = regex.match(line)
+ if m:
+ var, token = m.groups()
+ line = re.sub(token, flags[var], line)
Review Comment:
Sorry, minutes after I've suggested using re.sub here I changed to use string's replace() -- you read the comment too fast! :-) I think replace is better because token is not really a regex so it's clearer, hence could you use:
```
line = line.replace(token, flags[var])
```
instead of re.sub here, please?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a diff in pull request #13313: [microTVM][CRT] Add memory size as project option
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #13313:
URL: https://github.com/apache/tvm/pull/13313#discussion_r1016867451
##########
src/runtime/crt/host/microtvm_api_server.py:
##########
@@ -57,7 +71,14 @@ def server_info_query(self, tvm_version):
optional=["build"],
type="bool",
help="Run make with verbose output",
- )
+ ),
+ server.ProjectOption(
+ "memory_size_bytes",
+ optional=["generate_project"],
+ type="int",
+ default=None,
Review Comment:
sure, I'll add it. But what's annoying is that even if we set the default to be `MEMORY_SIZE_BYTES`, then later when we want to use `options["memory_size_bytes"]` we have to set the default value again by using something like: `options.get("memory_size_bytes", MEMORY_SIZE_BYTES)`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on a diff in pull request #13313: [microTVM][CRT]Add memory size as project option
Posted by GitBox <gi...@apache.org>.
gromero commented on code in PR #13313:
URL: https://github.com/apache/tvm/pull/13313#discussion_r1016000405
##########
src/runtime/crt/host/microtvm_api_server.py:
##########
@@ -57,7 +71,14 @@ def server_info_query(self, tvm_version):
optional=["build"],
type="bool",
help="Run make with verbose output",
- )
+ ),
+ server.ProjectOption(
+ "memory_size_bytes",
+ optional=["generate_project"],
+ type="int",
+ default=None,
Review Comment:
Shouldn't `MEMORY_SIZE_BYTES` be the default here?
##########
src/runtime/crt/host/microtvm_api_server.py:
##########
@@ -67,6 +88,29 @@ def server_info_query(self, tvm_version):
# The build target given to make
BUILD_TARGET = "build/main"
+ def _populate_makefile(
+ self,
+ makefile_template_path: pathlib.Path,
+ makefile_path: pathlib.Path,
+ memory_size: int,
Review Comment:
How about passing a dict (like the `flags` list) here instead of `memory_size`? I'm just wondering if new variables get added to the Makefile in the future. That also would avoids having an internal dict where we need to repeat options that affect the token resolution in the Makefile. But that's just a suggestion.
##########
src/runtime/crt/host/microtvm_api_server.py:
##########
@@ -67,6 +88,29 @@ def server_info_query(self, tvm_version):
# The build target given to make
BUILD_TARGET = "build/main"
+ def _populate_makefile(
+ self,
+ makefile_template_path: pathlib.Path,
+ makefile_path: pathlib.Path,
+ memory_size: int,
+ ):
+ """Generate Makefile from template."""
+ flags = {
+ "MEMORY_SIZE_BYTES": str(memory_size),
+ }
+
+ with open(makefile_path, "w") as makefile_f:
+ with open(makefile_template_path, "r") as makefile_template_f:
+ for line in makefile_template_f:
+ SUBST_TOKEN_RE = re.compile(r"<([A-Z_]+)>")
+ outs = []
+ for i, m in enumerate(re.split(SUBST_TOKEN_RE, line)):
+ if i % 2 == 1:
+ m = flags[m]
+ outs.append(m)
+ line = "".join(outs)
+ makefile_f.write(line)
Review Comment:
Could we make it shorter with (or something alike to):
```
token_val = {
"MEMORY": str(memory_size),
}
regex = re.compile(r'([A-Z_]+) := (<[A-Z_]+>)')
with open(makefile_path, "w") as makefile_f:
with open(makefile_template_path, "r") as makefile_template_f:
for line in makefile_template_f:
m = regex.match(line)
if m:
var, token = m.groups()
line = re.sub(token, token_val[var], line)
makefile_f.write(line)
```
?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on a diff in pull request #13313: [microTVM][CRT] Add memory size as project option
Posted by GitBox <gi...@apache.org>.
gromero commented on code in PR #13313:
URL: https://github.com/apache/tvm/pull/13313#discussion_r1016653327
##########
src/runtime/crt/host/microtvm_api_server.py:
##########
@@ -67,6 +80,27 @@ def server_info_query(self, tvm_version):
# The build target given to make
BUILD_TARGET = "build/main"
+ def _populate_makefile(
+ self,
+ makefile_template_path: pathlib.Path,
+ makefile_path: pathlib.Path,
+ memory_size: int,
+ ):
+ """Generate Makefile from template."""
+ flags = {
+ "MEMORY_SIZE_BYTES": str(memory_size),
+ }
+
+ regex = re.compile(r"([A-Z_]+) := (<[A-Z_]+>)")
+ with open(makefile_path, "w") as makefile_f:
+ with open(makefile_template_path, "r") as makefile_template_f:
+ for line in makefile_template_f:
+ m = regex.match(line)
+ if m:
+ var, token = m.groups()
+ line = re.sub(token, flags[var], line)
Review Comment:
Sorry, minutes after I've suggested using `re.sub` here I changed to use string's `replace()` -- you read the comment too fast! :-) I think `replace` is better because `token` is not really a regex so it's clearer, hence could you use:
```
line = line.replace(token, flags[var])
```
instead of `re.sub` here, please?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a diff in pull request #13313: [microTVM][CRT] Add memory size as project option
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #13313:
URL: https://github.com/apache/tvm/pull/13313#discussion_r1016908534
##########
src/runtime/crt/host/microtvm_api_server.py:
##########
@@ -57,7 +71,14 @@ def server_info_query(self, tvm_version):
optional=["build"],
type="bool",
help="Run make with verbose output",
- )
+ ),
+ server.ProjectOption(
+ "memory_size_bytes",
+ optional=["generate_project"],
+ type="int",
+ default=None,
Review Comment:
@gromero yeah that makes sense
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a diff in pull request #13313: [microTVM][CRT] Add memory size as project option
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #13313:
URL: https://github.com/apache/tvm/pull/13313#discussion_r1016042988
##########
src/runtime/crt/host/microtvm_api_server.py:
##########
@@ -67,6 +88,29 @@ def server_info_query(self, tvm_version):
# The build target given to make
BUILD_TARGET = "build/main"
+ def _populate_makefile(
+ self,
+ makefile_template_path: pathlib.Path,
+ makefile_path: pathlib.Path,
+ memory_size: int,
+ ):
+ """Generate Makefile from template."""
+ flags = {
+ "MEMORY_SIZE_BYTES": str(memory_size),
+ }
+
+ with open(makefile_path, "w") as makefile_f:
+ with open(makefile_template_path, "r") as makefile_template_f:
+ for line in makefile_template_f:
+ SUBST_TOKEN_RE = re.compile(r"<([A-Z_]+)>")
+ outs = []
+ for i, m in enumerate(re.split(SUBST_TOKEN_RE, line)):
+ if i % 2 == 1:
+ m = flags[m]
+ outs.append(m)
+ line = "".join(outs)
+ makefile_f.write(line)
Review Comment:
I changed it to this
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on pull request #13313: [microTVM][CRT] Add memory size as project option
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on PR #13313:
URL: https://github.com/apache/tvm/pull/13313#issuecomment-1306456117
@gromero PTAL
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org