-
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
Contributing the STM32 port #7742
Conversation
Thanks @stoa , given this is a new feature. It would be great if you can post an RFC thread |
hi @stoa, thanks for your contribution! I agree with @tqchen an RFC would be great. looking over the code I see a lot of concepts implemented that are fairly similar to what we have implemented in microTVM now:
For microTVM, we're taking the approach of reusing C runtime components between platforms and/or SoCs, and allowing platform-specific e.g. memory allocators by implementing It would be great if we could consider a way to unify your approach with our microTVM approach. It seems like the STM32 firmware code could become an implementation of our Project API (similar to the host demo project). This would allow us to bring further improvements to TVM currently under development (e.g. AOT runtime, graph-level optimization, Here are some pointers to RFCs that describe our approach in more detail:
Please also feel free to suggest improvements or changes to our current implementation. It is very much in development and we would like to encourage contributions! |
Hello @areusch and @tqchen . |
Hi Arthur! That would be great. Feel free to open a new topic categorized under Development > RFC on our discuss forum. If you look in that category you can see some examples of how people have titled and structured their RFCs. However, the process is fairly informal at this point. This PR would be a great PoC to backlink from the RFC as well, so people can read more specifics if they like. Looking forward to it! Thanks, |
Hello, Andrew |
@stoa please ping me when you want me to take another look here |
@areusch |
hi @stoa, try adding |
@stoa let me know when you're ready for another review |
@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.
@stoa thanks for fixing the test. could you address my may 24 comments on emitter.py? there are a bunch of style things we should fix and simplifications to the code to make it read like the rest of the TVM repo and therefore easier to maintain. inside apps we can have some flexibility, but code in python
should all follow similar convention. i've left a couple more comments as well.
include/tvm/runtime/c_runtime_api.h
Outdated
@@ -190,6 +190,7 @@ TVM_DLL void TVMAPISetLastError(const char* msg); | |||
* \return error info | |||
*/ | |||
TVM_DLL const char* TVMGetLastError(void); | |||
|
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.
ping on this one
python/tvm/contrib/stm32/emitter.py
Outdated
import tvm | ||
from tvm.contrib import utils | ||
|
||
AI_TOOLS_VERSION_MAJOR = 1 |
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.
ping on this one
python/tvm/contrib/stm32/emitter.py
Outdated
return node_name.replace(":", "_") | ||
|
||
|
||
# ========================================================== |
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.
are you guys ok with deleting the # =======
comments?
@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.
@stoa thanks for addressing those last comments. i just have a few more for this one and i think we are good to merge after we've addressed these. thanks for your patience here!
The quantization information for model inputs/outputs. | ||
""" | ||
|
||
for key in self.graph_: |
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.
from other file: i would suggest not extracting these from self.graph_ and instead just accessing them using self.graph_["nodes"]. it may be very slightly slower, but it makes the code easier to maintain as you don't need to keep track of where each of these properties comes from. if you want to assert there aren't unknown keys in the graph, you could assert(list(sorted(self.graph_)) == ["arg_nodes", "attrs", "heads", "metadata", "node_row_ptr", "nodes"])
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.
I prefer to keep a separate data structure for the emitter. Generally, this data structure is not necessarily equal to the TVM graph_['nodes']. For example, I may decide to not include OPs that are a 'noop' in the code, etc. This is done on purpose to simplify development. No problem maintaining - the structure is built in a single place from the graph.
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.
ok, i don't think we need to block on this but i don't see you doing any modification to self._nodes
, self._arg_nodes
, self._node_row_ptr
, self._outputs
, or self._attrs
right now.
@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.
@stoa sorry for the long delay. found a couple more unaddressed style things and suggested some small changes to make the tests more robust. otherwise, looks good now. thanks for working through these!
return size * _get_type_size(dltype) | ||
|
||
|
||
def _preprocess_code(src): |
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.
just curious: in general i think our code emitter should properly include all the stdlibs required to compile the code. i think maybe originally this included "ai_platform_api.h"
if my memory serves? just curious if you need this function still?
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.
Yes. Because math.h is missing (perhaps no longer missing in the newwest TVM), and because we have a CMSIS option that I will contribute later that needs more.
The quantization information for model inputs/outputs. | ||
""" | ||
|
||
for key in self.graph_: |
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.
ok, i don't think we need to block on this but i don't see you doing any modification to self._nodes
, self._arg_nodes
, self._node_row_ptr
, self._outputs
, or self._attrs
right now.
src = fin.read() | ||
src_files.append(src) | ||
|
||
self.graph_ = graph_dict |
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.
think there are a couple more style fixes (e.g. self._graph and self._lib)
model_h_name = os.path.join(dest_dir, model_name + ".h") | ||
model_c_name = os.path.join(dest_dir, model_name + ".c") | ||
|
||
data_h = open(data_h_name, "w") |
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.
could you do this like so:
import contextlib
with contextlib.ExitStack() as exit_stack:
data_h = exit_stack.enter_context(open(data_h_name, "w"))
data_c = exit_stack.enter_context(open(data_c_name, "w"))
out_h = exit_stack.enter_context(open(out_h_name, "w"))
out_c = exit_stack.enter_context(open(out_h_name, "w"))
self._emit_params_data(...)
self.emit_open(...)
# .. all other self._emit
# no need to close() the ExitStack does this for you
this way, if an exception occurs in any self._emit_*
, Python will auto-close the files for you.
def test_mnist(): | ||
curr_path = os.path.dirname(os.path.abspath(os.path.expanduser(__file__))) | ||
model_path = os.path.join(curr_path, "models/mnist.tflite") | ||
build_dir = os.path.join(curr_path, BUILD_DIR) |
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.
as we add more tests, we'll need to config filesystem modifications in a test-specific directory. can you do this:
DEBUG = False
tempdir_root = None
if DEBUG:
tempdir_root = os.path.join(curr_path, f"workspace", "test_mnist", datetime.datetime.now().strftime("%Y-%m-%dT%H-%M-%S"))
build_dir = tvm.contrib.utils.tempdir(tempdir_root)
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.
thanks @stoa for working through these issues! i think we can merge now!
* Contribute apps/stm32 application. * Removed a useless file. * STM32: Use Model Library Format in demo. Added test. * STM32: Added quantized mnist test. * STM32: Fixed apps/stm32/Makefile. * STM32: Added quantized models test. * STM32: Added tests to tests/scripts/task_python_microtvm.sh * STM32: Listed specific files with lint. * STM32: removed external copyright notices. * STM32: Added missing ASF copyright notice. * STM32: style fixes. * STM32: more style fixes. * STM32: fixed liny for C files. * STM32: Does extern C help with cpplint. * STM32: Fixed wrong LINT_C_FILE spelling. * STM32: Still some lint fixes. * STM32: more style. * STM32: More fixes lint+formatting. * STM32: cleanup. * STM32: style cleanup. * STM32: Moved ai_runner to the apps/stm32. * Alignment with PR 7742 * lint cleanup. * STM32: Use crt_backend_api.c with standalone build. * STM32: Fixed the CI test. * STM32: style fixes. * STM32: Removed unused files. * STM32: Moved to crt_backend_api * STM32: style fix. * STM32: style fix. * Revert "STM32: Removed unused files." This reverts commit d72f8e5. Undo changes to c_backend_api/c_runtime_api. * Revert "STM32: Moved to crt_backend_api" This reverts commit 6c0e666. Undo changes to the c_backend-api/c_runtime_api. * stm32: aligned to micro TVM structure. * stm32: improved the python style. * stm32: cpplint fixes. * stm32: Fixed the test * stm32: style fixes. * stm32: style fixes. * stm32: style fixes. * stm32: style fixes.
* Contribute apps/stm32 application. * Removed a useless file. * STM32: Use Model Library Format in demo. Added test. * STM32: Added quantized mnist test. * STM32: Fixed apps/stm32/Makefile. * STM32: Added quantized models test. * STM32: Added tests to tests/scripts/task_python_microtvm.sh * STM32: Listed specific files with lint. * STM32: removed external copyright notices. * STM32: Added missing ASF copyright notice. * STM32: style fixes. * STM32: more style fixes. * STM32: fixed liny for C files. * STM32: Does extern C help with cpplint. * STM32: Fixed wrong LINT_C_FILE spelling. * STM32: Still some lint fixes. * STM32: more style. * STM32: More fixes lint+formatting. * STM32: cleanup. * STM32: style cleanup. * STM32: Moved ai_runner to the apps/stm32. * Alignment with PR 7742 * lint cleanup. * STM32: Use crt_backend_api.c with standalone build. * STM32: Fixed the CI test. * STM32: style fixes. * STM32: Removed unused files. * STM32: Moved to crt_backend_api * STM32: style fix. * STM32: style fix. * Revert "STM32: Removed unused files." This reverts commit d72f8e5. Undo changes to c_backend_api/c_runtime_api. * Revert "STM32: Moved to crt_backend_api" This reverts commit 6c0e666. Undo changes to the c_backend-api/c_runtime_api. * stm32: aligned to micro TVM structure. * stm32: improved the python style. * stm32: cpplint fixes. * stm32: Fixed the test * stm32: style fixes. * stm32: style fixes. * stm32: style fixes. * stm32: style fixes.
We propose to contribute the STM32 port of the TVM.
The STM32 are mainly bare-metal boards and often use-cases do not suppose using an OS: Linux, embed,RTOS,etc.
The STM32 development tools (ex, CubeMX) build standalone applications (no OS) that can be flashed to the board and executed.
In this TVM port, the TVM is used to compile AI networks into a C code compliant with the STM32 AI API.
In particular, the TVM is used to build a C module (kernels library + JSON graph + params); then the stm32.emitter.py generates the C implementation from the TVM module's JSON graph and writes out a C implementation of the kernels.
The generated C code can then be included as part of a larger STM32 application, and managed within a standard STM32 application development flow.
This contribution includes:
Tested on development board with several models including mnist, inceprion, squeezenet, mobilenet, yolo, and a number of proprietary models.
This contribution does not modify any of the original TVM files.
@areusch
@tqchen