-
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
[µTVM] Add --runtime=c, remove micro_dev target, enable LLVM backend #6145
Conversation
apps/bundle_deploy/bundle_static.c
Outdated
if (nptrs < 0) { | ||
perror("backtracing"); | ||
} else { | ||
backtrace_symbols_fd(trace, nptrs, STDOUT_FILENO); |
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.
This is a great feature to have backtrace enabled, however, since these functions are not part of the standard c programming language, and it would have potential portability issue, can we add a macro to disable this feature for the platforms where backtracing is not supported?
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'm actually just trying to use this to debug the regression, I couldn't reproduce the gpu failure on my side. but, now i've found a node where i should be able to run tvmai/ci-gpu, so i'm going to take that approach.
for merging, I don't know--I know we had issues with the CRT being flaky in the regression before. it might be worth it to leave this code in, since flaky test failures are almost always solved with more data. so, i'd be happy to put it behind a #define
and enable that for the regression, if you're also happy with that?
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, this is exactly what I wanted from the previous comment, and I think leaving backtracing flag on should be fine.
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.
okay, added a Makefile flag to control this.
test_dwarf_debug_information() | ||
test_llvm_shuffle() | ||
test_llvm_bf16() | ||
# test_multiple_func() |
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.
did you disable these unintentionally?
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.
whoops, after the flaky CI CUDA issue is resolved i'll push a patch with this reverted
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.
uncommented
I think this is now ready for review (CI failure is due to an unrelated flake, I believe). |
The CI error seems unrelated, please rebase to trigger it again. |
apps/bundle_deploy/Makefile
Outdated
@mkdir -p $(@D) | ||
gcc $(PKG_CFLAGS) -o $@ $^ | ||
gcc $(PKG_CFLAGS) -o $@ $^ -ldl |
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 think linking to dl
library within a static target isn't the desired intention. @mehrdadh what's your take on this?
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.
it's needed for the backtrace logic, so wherever that was used.
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.
Left a minor comment, LGTM overall.
@@ -22,7 +22,11 @@ | |||
#include <tvm/runtime/crt/crt.h> | |||
#include <tvm/runtime/crt/graph_runtime.h> | |||
#include <tvm/runtime/crt/packed_func.h> | |||
#include <unistd.h> |
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 do we need this header file here?
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.
Ah I think I missed this one, it’s probably not needed. But, I won’t be able to address this for a week, so perhaps we can merge as is? Happy to take an issue to fix in a followup PR if that’s ok with you.
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 suggest this gets trimmed in a follow up PR
As part of this PR there is a small change to tvm/tests/micro/test_runtime_micro_on_arm.py, attempting to run the tests fail. |
Since there isn't an RFC, I'll drop this comment here, what does a somewhat working example look like? Updating the micro tvm tutorial as inspiration : target = "c --system-lib --runtime=c" dev_config = micro.device.arm.stm32f746xx.generate_config("127.0.0.1", 6666) mod, params = relay.frontend.from_tflite(tflite_model, with micro.Session(dev_config) as sess:
This feels like it should be close however it fails at micro.create_micro_mod /tmp/tmpok80dx2r/temp.c:232:5: warning: initialization of 'int (*)(TVMValue *, int *, int, TVMValue *, int *, void )' {aka 'int ()(union *, int *, int, union *, int *, void )'} from incompatible pointer type 'int32_t ()(void *, void *, int32_t, void *, void *, void )' {aka 'long int ()(void *, void *, long int, void *, void *, void )'} [-Wincompatible-pointer-types] Should that be expected for the time being? |
@tom-gall I inspected the generated C from the example code you posted, and I'm seeing this at the end of the file:
static TVMBackendPackedCFunc _tvm_func_array[] = {
fused_reshape,
fused_reshape_1,
fused_nn_dense_nn_bias_add,
fused_nn_dense_nn_bias_add_nn_relu,
fused_nn_dense_nn_bias_add_nn_relu_1,
static const TVMFuncRegistry _tvm_func_registry = {
"\484849fused_reshape\484848fused_reshape_1\484848fused_nn_dense_nn_bias_add\484848fused_nn_dense_nn_bias_..."
};
static const TVMModule _tvm_system_lib = {
&system_lib_registry,
};
const TVMModule* TVMSystemLibEntryPoint(void) {
return &system_lib;
} So my guess is there's a closing brace not being generated here. @areusch Maybe we should add a test that compiles the module source? |
…pache#6145) * need to fill address of globals in tvmfuncregistry * llvm func registry generator works! * lint fixes * rm hexdump include * bring bundle_deploy back to life and add to CI * revert gcda additions * git-clang-format * fix check for --system-lib and test_runtime_micro target * fixup compile flags for bundle_deploy CRT and improve robustness * git-clang-format * add debugging info * git-clang-format * initialize ret_values in PackedFunc_Call. * retrigger CI * fix log messages * git-clang-format * remove default for --runtime target opt * put backtrace behind a flag and enable it * simpify ReadString(), fixing bad instruction exception on os x. * git-clang-format * uncomment tests * reorder backtrace ldflags for linux gcc
…pache#6145) * need to fill address of globals in tvmfuncregistry * llvm func registry generator works! * lint fixes * rm hexdump include * bring bundle_deploy back to life and add to CI * revert gcda additions * git-clang-format * fix check for --system-lib and test_runtime_micro target * fixup compile flags for bundle_deploy CRT and improve robustness * git-clang-format * add debugging info * git-clang-format * initialize ret_values in PackedFunc_Call. * retrigger CI * fix log messages * git-clang-format * remove default for --runtime target opt * put backtrace behind a flag and enable it * simpify ReadString(), fixing bad instruction exception on os x. * git-clang-format * uncomment tests * reorder backtrace ldflags for linux gcc
…pache#6145) * need to fill address of globals in tvmfuncregistry * llvm func registry generator works! * lint fixes * rm hexdump include * bring bundle_deploy back to life and add to CI * revert gcda additions * git-clang-format * fix check for --system-lib and test_runtime_micro target * fixup compile flags for bundle_deploy CRT and improve robustness * git-clang-format * add debugging info * git-clang-format * initialize ret_values in PackedFunc_Call. * retrigger CI * fix log messages * git-clang-format * remove default for --runtime target opt * put backtrace behind a flag and enable it * simpify ReadString(), fixing bad instruction exception on os x. * git-clang-format * uncomment tests * reorder backtrace ldflags for linux gcc
…pache#6145) * need to fill address of globals in tvmfuncregistry * llvm func registry generator works! * lint fixes * rm hexdump include * bring bundle_deploy back to life and add to CI * revert gcda additions * git-clang-format * fix check for --system-lib and test_runtime_micro target * fixup compile flags for bundle_deploy CRT and improve robustness * git-clang-format * add debugging info * git-clang-format * initialize ret_values in PackedFunc_Call. * retrigger CI * fix log messages * git-clang-format * remove default for --runtime target opt * put backtrace behind a flag and enable it * simpify ReadString(), fixing bad instruction exception on os x. * git-clang-format * uncomment tests * reorder backtrace ldflags for linux gcc
…pache#6145) * need to fill address of globals in tvmfuncregistry * llvm func registry generator works! * lint fixes * rm hexdump include * bring bundle_deploy back to life and add to CI * revert gcda additions * git-clang-format * fix check for --system-lib and test_runtime_micro target * fixup compile flags for bundle_deploy CRT and improve robustness * git-clang-format * add debugging info * git-clang-format * initialize ret_values in PackedFunc_Call. * retrigger CI * fix log messages * git-clang-format * remove default for --runtime target opt * put backtrace behind a flag and enable it * simpify ReadString(), fixing bad instruction exception on os x. * git-clang-format * uncomment tests * reorder backtrace ldflags for linux gcc
…pache#6145) * need to fill address of globals in tvmfuncregistry * llvm func registry generator works! * lint fixes * rm hexdump include * bring bundle_deploy back to life and add to CI * revert gcda additions * git-clang-format * fix check for --system-lib and test_runtime_micro target * fixup compile flags for bundle_deploy CRT and improve robustness * git-clang-format * add debugging info * git-clang-format * initialize ret_values in PackedFunc_Call. * retrigger CI * fix log messages * git-clang-format * remove default for --runtime target opt * put backtrace behind a flag and enable it * simpify ReadString(), fixing bad instruction exception on os x. * git-clang-format * uncomment tests * reorder backtrace ldflags for linux gcc
This PR adds a
--runtime
attr to thec
andllvm
targets.--runtime
is expected to bec
orc++
currently, and this option specifies the way in which generated modules handle function lookup. in the c++ case, global initializer functions may be called (system lib) or the symbol table may be consulted bylibrary_module.cc
. in the c case, a FuncRegistry is generated and an explicit initialization hookTVMSystemLibEntryPoint
is defined.currently, this PR only supports generating system libs for the C runtime, but the functionality is split into two pieces so that it's clear what would need to be addressed to support a form of dynamically-linked C libs. much of this PR is written assuming a bare metal environment (explicit initializer, function lookup table in flash, no presumed use of dlopen, etc), so a "dynamically-linked" solution would ideally support that environment as well.
this PR is in service of MISRA-C changes for RPC support RFC; if you think it deserves its own RFC, I'm happy to write one up.