-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[microTVM] Zephyr: RISCV support for Zephyr QEMU RISCV-32/64 #7804
Conversation
cc @areusch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work @mehrdadh ! just a couple small things to address.
apps/microtvm/zephyr/demo_runtime/qemu-hack/qemu-system-riscv64
Outdated
Show resolved
Hide resolved
BTW @mehrdadh here are some git tips that might be useful(try to rebase to bring the code back instead of merge) https://tvm.apache.org/docs/contribute/git_howto.html |
@tqchen Thanks for the tips! I will follow the guideline. |
@tqchen, this method unfortunately makes the review much harder because GH loses the old commits after force-push. If you're trying to decide whether an open comment has been addressed, it's really helpful to be able to be able to see the context which generated that comment and a diff. I think the strategy adopted by me (and a few others) has been to rebase and force-push until a comment is placed on the PR, then do merges. If we want to consider improving the situation, I think we unfortunately may have to look into a different code review tool e.g. Gerrit (unless I misunderstand repository config settings on GH). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry couple more small things
@@ -59,7 +66,7 @@ def _make_sess_from_op(model, zephyr_board, west_cmd, op_name, sched, arg_bufs): | |||
|
|||
|
|||
def _make_session(model, target, zephyr_board, west_cmd, mod): | |||
test_name = f"{os.path.splitext(os.path.abspath(__file__))[0]}_{model}" | |||
test_name = f"{os.path.splitext(os.path.abspath(__file__))[0]}_{zephyr_board}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change? seems like we should put both in the path, if anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@areusch so we distinguish the test based on the zephyr_board. For ex. qemu_x86, qemu_riscv32 and qemu_riscv64 all use the same model
which is host
, but they have different zephyr_board. zephyr_board will always be unique.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sorry, I was misreading. this looks good
@@ -59,7 +66,7 @@ def _make_sess_from_op(model, zephyr_board, west_cmd, op_name, sched, arg_bufs): | |||
|
|||
|
|||
def _make_session(model, target, zephyr_board, west_cmd, mod): | |||
test_name = f"{os.path.splitext(os.path.abspath(__file__))[0]}_{model}" | |||
test_name = f"{os.path.splitext(os.path.abspath(__file__))[0]}_{zephyr_board}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sorry, I was misreading. this looks good
…7804) * working on qemu * debugging * riscv hacks * config added * change target platforms * fix merge * debugging issue with zephyr 2.5 * cleanup * working on qemu * debugging * riscv hacks * config added * change target platforms * fix merge * debugging issue with zephyr 2.5 * cleanup * testing * pass riscv64 * fix merge * small fix * update vm_name * add zephyr version * add comment for riscv32 issue * remove debug messages * cleanup * cleanup * change workspace * fix zephyr version * cleanup * change to symlink * fix flag * add comment * lint check * lint fix * fix format * rename debugger * rework args Co-authored-by: Mehrdad Hessar <[email protected]> Co-authored-by: Andrew Reusch <[email protected]>
…7804) * working on qemu * debugging * riscv hacks * config added * change target platforms * fix merge * debugging issue with zephyr 2.5 * cleanup * working on qemu * debugging * riscv hacks * config added * change target platforms * fix merge * debugging issue with zephyr 2.5 * cleanup * testing * pass riscv64 * fix merge * small fix * update vm_name * add zephyr version * add comment for riscv32 issue * remove debug messages * cleanup * cleanup * change workspace * fix zephyr version * cleanup * change to symlink * fix flag * add comment * lint check * lint fix * fix format * rename debugger * rework args Co-authored-by: Mehrdad Hessar <[email protected]> Co-authored-by: Andrew Reusch <[email protected]>
…7804) * working on qemu * debugging * riscv hacks * config added * change target platforms * fix merge * debugging issue with zephyr 2.5 * cleanup * working on qemu * debugging * riscv hacks * config added * change target platforms * fix merge * debugging issue with zephyr 2.5 * cleanup * testing * pass riscv64 * fix merge * small fix * update vm_name * add zephyr version * add comment for riscv32 issue * remove debug messages * cleanup * cleanup * change workspace * fix zephyr version * cleanup * change to symlink * fix flag * add comment * lint check * lint fix * fix format * rename debugger * rework args Co-authored-by: Mehrdad Hessar <[email protected]> Co-authored-by: Andrew Reusch <[email protected]>
…7804) * working on qemu * debugging * riscv hacks * config added * change target platforms * fix merge * debugging issue with zephyr 2.5 * cleanup * working on qemu * debugging * riscv hacks * config added * change target platforms * fix merge * debugging issue with zephyr 2.5 * cleanup * testing * pass riscv64 * fix merge * small fix * update vm_name * add zephyr version * add comment for riscv32 issue * remove debug messages * cleanup * cleanup * change workspace * fix zephyr version * cleanup * change to symlink * fix flag * add comment * lint check * lint fix * fix format * rename debugger * rework args Co-authored-by: Mehrdad Hessar <[email protected]> Co-authored-by: Andrew Reusch <[email protected]>
…7804) * working on qemu * debugging * riscv hacks * config added * change target platforms * fix merge * debugging issue with zephyr 2.5 * cleanup * working on qemu * debugging * riscv hacks * config added * change target platforms * fix merge * debugging issue with zephyr 2.5 * cleanup * testing * pass riscv64 * fix merge * small fix * update vm_name * add zephyr version * add comment for riscv32 issue * remove debug messages * cleanup * cleanup * change workspace * fix zephyr version * cleanup * change to symlink * fix flag * add comment * lint check * lint fix * fix format * rename debugger * rework args Co-authored-by: Mehrdad Hessar <[email protected]> Co-authored-by: Andrew Reusch <[email protected]>
This PR adds following changes:
Note: test_zephyr.py fully passes on qemu_riscv64 using the zephyr 2.5. However, for qemu_riscv32 you need to update zephyr to latest master branch. This issue is mentioned on zephyr: zephyrproject-rtos/zephyr#34026
We will update this when we update our zephyr to next version.