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/04/09 03:39:47 UTC
[GitHub] [tvm] gromero opened a new pull request #7813: [µTVM] Add support for mps2_an521 board
gromero opened a new pull request #7813:
URL: https://github.com/apache/tvm/pull/7813
Hi,
Could the following patchset be reviewed please?
It enables the Arm MPS2-AN521 board be used on microTVM:
TARGET = tvm.target.target.micro("mps2_an521")
BOARD = "mps2_an521-qemu"
It also adds to `micro_tflite.py` an example on how to run Zephyr demo in apps/ using that new target.
To use the MPS2-AN521 target it's necessary the newest Zephyr 2.5.0 branch [0] that contains the following fixes:
drivers: uart: uart_cmsdk_apb: fix interrupt handling
https://github.com/zephyrproject-rtos/zephyr/commit/ec0aa8331a652b7e7ed5d9c5653d6d70c3789dc6
drivers: uart: uart_cmsdk_apb: Fix uart_irq_is_pending
https://github.com/zephyrproject-rtos/zephyr/commit/bd0fe17fa0161e28405ec49d8da0f4ca9db90723
boards: arm: mps2-an521: Fix DT memory regions
https://github.com/zephyrproject-rtos/zephyr/commit/fe3f71bdad8b92ff3f559f843dc43cb45373f0a4
It's also necessary to have `qemu-system-arm` available in the environment (reachable from $PATH).
Thanks,
Gustavo
[0] https://github.com/zephyrproject-rtos/zephyr/commits/v2.5-branch
--
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 a change in pull request #7813: [µTVM] Add support for mps2_an521 board
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #7813:
URL: https://github.com/apache/tvm/pull/7813#discussion_r610906010
##########
File path: python/tvm/micro/contrib/zephyr.py
##########
@@ -108,7 +108,18 @@ def __init__(
f"project_dir supplied to ZephyrCompiler does not exist: {project_dir}"
)
+ if "qemu" in board:
Review comment:
@areusch I can but I'm actually trimming off based on that check too. If I simplify like you suggest it would be:
```
diff --git a/python/tvm/micro/contrib/zephyr.py b/python/tvm/micro/contrib/zephyr.py
index ed66ce111..e772fa610 100644
--- a/python/tvm/micro/contrib/zephyr.py
+++ b/python/tvm/micro/contrib/zephyr.py
@@ -108,15 +108,14 @@ class ZephyrCompiler(tvm.micro.Compiler):
f"project_dir supplied to ZephyrCompiler does not exist: {project_dir}"
)
- if "qemu" in board:
- self._qemu = True
+ self._qemu = "qemu" in board
- # For Zephyr boards that run emulated by default, i.e. "simulation: qemu" is set in
- # the board's .yaml config file, and which their names don't have the prefix "qemu_", a
- # suffix "-qemu" is used in microTVM to inform that the QEMU transporter has to be used.
- # So the suffix needs to be trimmed off before the board name is passed to Zephyr.
- if "-qemu" in board:
- board = board.replace("-qemu", "")
+ # For Zephyr boards that run emulated by default, i.e. "simulation: qemu" is set in
+ # the board's .yaml config file, and which their names don't have the prefix "qemu_", a
+ # suffix "-qemu" is used in microTVM to inform that the QEMU transporter has to be used.
+ # So the suffix needs to be trimmed off before the board name is passed to Zephyr.
+ if "-qemu" in board:
+ board = board.replace("-qemu", "")
self._board = board
```
i.e. to keep the same result I would have to check for "-qemu" anyway below in order to trim off the suffix from the board name. Am I following your suggestion correctly? If so, I still prefer trimming off only when "qemu" is in board, so the first version.
--
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] mehrdadh commented on pull request #7813: [µTVM] Add support for mps2_an521 board
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #7813:
URL: https://github.com/apache/tvm/pull/7813#issuecomment-816828794
@gromero Thanks for this PR!
For clarification, is Zephyr 2.5.0 branch [0] different than 2.5.0?
--
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 #7813: [µTVM] Add support for mps2_an521 board
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #7813:
URL: https://github.com/apache/tvm/pull/7813#issuecomment-819040829
thanks @gromero , the PR is now 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] areusch merged pull request #7813: [µTVM] Add support for mps2_an521 board
Posted by GitBox <gi...@apache.org>.
areusch merged pull request #7813:
URL: https://github.com/apache/tvm/pull/7813
--
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 #7813: [µTVM] Add support for mps2_an521 board
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #7813:
URL: https://github.com/apache/tvm/pull/7813#issuecomment-817978760
> @maheshambule @areusch Thanks a lot for the hint on how to check lint locally. I was not aware of them : ) `make format` actually suggested more than 200 changes, which really seemed too picky and not necessary, so I ended up using just `docker/bash.sh tlcpack/ci-lint:v0.62 ./tests/scripts/task_lint.s` and it made the magic.
yeah so this is why you should prefer `docker/lint.sh` (or equivalent `docker/bash.sh`) for linting operations--the output of the lint tool is extremely sensitive to the version of e.g. `cpplint`, `pylint`, etc that you are using. the docker container removes that issue from consideration.
--
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 #7813: [µTVM] Add support for mps2_an521 board
Posted by GitBox <gi...@apache.org>.
gromero commented on pull request #7813:
URL: https://github.com/apache/tvm/pull/7813#issuecomment-819070724
> thanks @gromero , the PR is now merged!
@areusch Thanks a lot for the reviews and for merging 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
[GitHub] [tvm] gromero commented on pull request #7813: [µTVM] Add support for mps2_an521 board
Posted by GitBox <gi...@apache.org>.
gromero commented on pull request #7813:
URL: https://github.com/apache/tvm/pull/7813#issuecomment-817013111
> @gromero Thanks for this PR!
> For clarification, is Zephyr 2.5.0 branch [0] different than 2.5.0?
Hi @mehrdadh ! Yes, they are different. The `v2.5.0` is a tag and so a cut of `master` when the release happened. The `v2.5-branch` is a maintenance branch where the backports land (the great majority are fixes) for 2.5.0 and, afaik, it's considered stable at any time. I pointed out that this PR requires the `v2.5-branch` because the fixes I've listed (that are necessary for the MPS2-AN521) landed in `master` after the "cut" for the `v2.5.0` release, so I had to backport them to Zephyr 2.5.0, hence they are present in `v2.5-branch` only.
--
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 a change in pull request #7813: [µTVM] Add support for mps2_an521 board
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #7813:
URL: https://github.com/apache/tvm/pull/7813#discussion_r610906010
##########
File path: python/tvm/micro/contrib/zephyr.py
##########
@@ -108,7 +108,18 @@ def __init__(
f"project_dir supplied to ZephyrCompiler does not exist: {project_dir}"
)
+ if "qemu" in board:
Review comment:
@areusch I can but I'm actually trimming off based on that check too. If I simplify like you suggest it would be:
```
diff --git a/python/tvm/micro/contrib/zephyr.py b/python/tvm/micro/contrib/zephyr.py
index ed66ce111..e772fa610 100644
--- a/python/tvm/micro/contrib/zephyr.py
+++ b/python/tvm/micro/contrib/zephyr.py
@@ -108,15 +108,14 @@ class ZephyrCompiler(tvm.micro.Compiler):
f"project_dir supplied to ZephyrCompiler does not exist: {project_dir}"
)
- if "qemu" in board:
- self._qemu = True
+ self._qemu = "qemu" in board
- # For Zephyr boards that run emulated by default, i.e. "simulation: qemu" is set in
- # the board's .yaml config file, and which their names don't have the prefix "qemu_", a
- # suffix "-qemu" is used in microTVM to inform that the QEMU transporter has to be used.
- # So the suffix needs to be trimmed off before the board name is passed to Zephyr.
- if "-qemu" in board:
- board = board.replace("-qemu", "")
+ # For Zephyr boards that run emulated by default, i.e. "simulation: qemu" is set in
+ # the board's .yaml config file, and which their names don't have the prefix "qemu_", a
+ # suffix "-qemu" is used in microTVM to inform that the QEMU transporter has to be used.
+ # So the suffix needs to be trimmed off before the board name is passed to Zephyr.
+ if "-qemu" in board:
+ board = board.replace("-qemu", "")
self._board = board
```
i.e. to keep the same result I would have to check for "-qemu" anyway below in order to trim off the suffix from the board name. Am I following your suggestion correctly? If so, I still prefer checking for the need of trimming off only when "qemu" is in board name, so the first version.
--
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 #7813: [µTVM] Add support for mps2_an521 board
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7813:
URL: https://github.com/apache/tvm/pull/7813#discussion_r610818942
##########
File path: python/tvm/micro/contrib/zephyr.py
##########
@@ -108,7 +108,18 @@ def __init__(
f"project_dir supplied to ZephyrCompiler does not exist: {project_dir}"
)
+ if "qemu" in board:
Review comment:
can you simplify: self._is_qemu = "qemu" in board
--
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 a change in pull request #7813: [µTVM] Add support for mps2_an521 board
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #7813:
URL: https://github.com/apache/tvm/pull/7813#discussion_r610906010
##########
File path: python/tvm/micro/contrib/zephyr.py
##########
@@ -108,7 +108,18 @@ def __init__(
f"project_dir supplied to ZephyrCompiler does not exist: {project_dir}"
)
+ if "qemu" in board:
Review comment:
@areusch I can but I'm actually trimming off based on that check too. If I simplify like you suggest it would be:
```
diff --git a/python/tvm/micro/contrib/zephyr.py b/python/tvm/micro/contrib/zephyr.py
index ed66ce111..e772fa610 100644
--- a/python/tvm/micro/contrib/zephyr.py
+++ b/python/tvm/micro/contrib/zephyr.py
@@ -108,15 +108,14 @@ class ZephyrCompiler(tvm.micro.Compiler):
f"project_dir supplied to ZephyrCompiler does not exist: {project_dir}"
)
- if "qemu" in board:
- self._qemu = True
+ self._qemu = "qemu" in board
- # For Zephyr boards that run emulated by default, i.e. "simulation: qemu" is set in
- # the board's .yaml config file, and which their names don't have the prefix "qemu_", a
- # suffix "-qemu" is used in microTVM to inform that the QEMU transporter has to be used.
- # So the suffix needs to be trimmed off before the board name is passed to Zephyr.
- if "-qemu" in board:
- board = board.replace("-qemu", "")
+ # For Zephyr boards that run emulated by default, i.e. "simulation: qemu" is set in
+ # the board's .yaml config file, and which their names don't have the prefix "qemu_", a
+ # suffix "-qemu" is used in microTVM to inform that the QEMU transporter has to be used.
+ # So the suffix needs to be trimmed off before the board name is passed to Zephyr.
+ if "-qemu" in board:
+ board = board.replace("-qemu", "")
self._board = board
```
i.e. to keep the same result I would have to check for "-qemu" anyway below in order to trim off the suffix from the board name. Am I following your suggestion correctly? IF so, I still prefer trimming off only when "qemu" is in board, so the first version.
--
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 #7813: [µTVM] Add support for mps2_an521 board
Posted by GitBox <gi...@apache.org>.
gromero commented on pull request #7813:
URL: https://github.com/apache/tvm/pull/7813#issuecomment-816975126
@maheshambule @areusch Thanks a lot for the hint on how to check lint locally. I was not aware of them : ) `make format` actually suggested more than 200 changes, which really seemed too picky and not necessary, so I ended up using just `docker/bash.sh tlcpack/ci-lint:v0.62 ./tests/scripts/task_lint.s` and it made the magic.
--
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] mehrdadh commented on pull request #7813: [µTVM] Add support for mps2_an521 board
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #7813:
URL: https://github.com/apache/tvm/pull/7813#issuecomment-816829376
@gromero there is a lint issue. You can run `make format` in the main directory and it will fix it assuming you have clang-format-10.
--
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 #7813: [µTVM] Add support for mps2_an521 board
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7813:
URL: https://github.com/apache/tvm/pull/7813#discussion_r612763650
##########
File path: python/tvm/micro/contrib/zephyr.py
##########
@@ -108,7 +108,18 @@ def __init__(
f"project_dir supplied to ZephyrCompiler does not exist: {project_dir}"
)
+ if "qemu" in board:
Review comment:
ah okay. your logic makes sense, but I guess now we have a passing PR and I think they're pretty similar. let's merge 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on a change in pull request #7813: [µTVM] Add support for mps2_an521 board
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #7813:
URL: https://github.com/apache/tvm/pull/7813#discussion_r611943142
##########
File path: python/tvm/micro/contrib/zephyr.py
##########
@@ -108,7 +108,18 @@ def __init__(
f"project_dir supplied to ZephyrCompiler does not exist: {project_dir}"
)
+ if "qemu" in board:
Review comment:
Over time I think that code will be updated to take care of FVP emulation too (about to be added to Zephyr, for Cortex M-55 + Ethos U-55, i.e. for MPS3 boards which are not supported by QEMU).
That said, I was actually not thinking of saving CPU cycles (I don't think there is a difference in performance between the two forms), nor about adding FVP support in the future, rather I just think that the first form is better to read/debug because it makes explicit that the trimming off applies only for a specific case where the suffix is present in the board name :) In the second form that dependency is "weak" and one maybe need to go back to the first `if` to realize that the second `if` will never be taken if the first is not taken. Anyway, since the result is the same, I'm sending a new version with the change suggested. Thanks.
--
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 #7813: [µTVM] Add support for mps2_an521 board
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7813:
URL: https://github.com/apache/tvm/pull/7813#discussion_r611807263
##########
File path: python/tvm/micro/contrib/zephyr.py
##########
@@ -108,7 +108,18 @@ def __init__(
f"project_dir supplied to ZephyrCompiler does not exist: {project_dir}"
)
+ if "qemu" in board:
Review comment:
your diff looks right to me. I think this is one of those cases where there is not much CPU time to be saved, and in particular since this is Python, the `self._is_qemu` lookup could cost more time than the substring check. in these cases, I just favor the easiest code to read/debug, rather than adding e.g. `if self._is_qemu and "-qemu" in board` (you would the have to explain the self._is_qemu--is this intending to capture some other criteria, which may shift over time to be more than just `"qemu" in board`?).
--
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 #7813: [µTVM] Add support for mps2_an521 board
Posted by GitBox <gi...@apache.org>.
gromero commented on pull request #7813:
URL: https://github.com/apache/tvm/pull/7813#issuecomment-816378917
@areusch @mehrdadh @tom-gall @leandron
--
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