[GitHub] [tvm] gromero commented on a diff in pull request #13313: [microTVM][CRT]Add memory size as project option

[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