Skip to content

Conversation

@mehrdadh
Copy link
Member

This PR:

  1. refactors crt_config.h generation to a single template file and use CMake configure_file func to generate in build
  2. Refactor Zephyr.cmake, CRT.cmake and Arduino.cmake to Micro.cmake

@tvm-bot
Copy link
Collaborator

tvm-bot commented Feb 10, 2023

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 by @-ing them in a comment.

Generated by tvm-bot

@mehrdadh mehrdadh force-pushed the micro/refactor_crt_config branch 2 times, most recently from a44ddd3 to bc1d597 Compare February 13, 2023 21:56
@mehrdadh mehrdadh marked this pull request as ready for review February 13, 2023 21:56
@mehrdadh mehrdadh force-pushed the micro/refactor_crt_config branch from bc1d597 to 788373c Compare February 13, 2023 21:58
@mehrdadh
Copy link
Member Author

cc @alanmacd @mkatanbaf for review

@mehrdadh mehrdadh force-pushed the micro/refactor_crt_config branch 2 times, most recently from 3963745 to 3b30d57 Compare February 14, 2023 18:23
/*!
* \file tvm/runtime/crt_config.h.template
* \brief Template for CRT configuration, to be modified on each target.
/*!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: something happened with spacing and slashes here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed this

.gitignore Outdated

# GDB history file
.gdb_history

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it makes sense to output these files into the build/ directory and add that directory as an include instead of putting these files into the source tree?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I moved all the crt_config.h files under apps/microtvm, just need to figure out the file under src/runtime/micro directory.

if (g_num_bytes_in_rx_buffer > RING_BUF_SIZE_BYTES) {
TVMPlatformAbort((tvm_crt_error_t)0xbeef3);
}
// if (g_num_bytes_in_rx_buffer > RING_BUF_SIZE_BYTES) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete commented code if not needed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for catching this, uncommented it.

import logging
from tvm.relay.backend import Runtime
from tvm.contrib import cc as _cc
from tvm.micro import copy_crt_config_header
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this add a dependency on USE_MICRO being enabled that wasn't here previously, should this be conditional on that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these tests were already executed with the assumption of USE_MICRO=ON
check this file: tests/scripts/task_microtvm_cpp_tests.sh

@mehrdadh mehrdadh force-pushed the micro/refactor_crt_config branch 2 times, most recently from 9650e62 to 9efb455 Compare February 14, 2023 23:17
@mehrdadh mehrdadh force-pushed the micro/refactor_crt_config branch from 9efb455 to 95131c3 Compare February 15, 2023 00:32
@mehrdadh mehrdadh force-pushed the micro/refactor_crt_config branch from 95131c3 to da9f700 Compare February 15, 2023 00:33
@mehrdadh mehrdadh requested a review from alanmacd February 15, 2023 00:33
@mehrdadh mehrdadh force-pushed the micro/refactor_crt_config branch from 4e0e6fe to d5bc32a Compare February 15, 2023 17:27
@mehrdadh mehrdadh force-pushed the micro/refactor_crt_config branch from d5bc32a to 900a023 Compare February 15, 2023 17:27
Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @mehrdadh !

@mehrdadh mehrdadh merged commit 1e5a830 into apache:main Feb 15, 2023
@mehrdadh mehrdadh deleted the micro/refactor_crt_config branch February 15, 2023 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants