-
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
Changes from 12 commits
2502874
88e0fe3
bc8c3a3
73e0fdc
15c2a2f
6971d41
c678370
8d9c6c7
7f52395
9b9d1d8
ef96809
c3c51a6
d91b2c2
3727f58
e429bd7
6c2cc72
c200ede
1551001
9550913
b431915
fcfdbbb
f9ca334
9150bce
0e5a8e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,11 +17,15 @@ | |
* under the License. | ||
*/ | ||
|
||
#define _GNU_SOURCE | ||
#include <dlfcn.h> | ||
#include <execinfo.h> | ||
#include <stdio.h> | ||
#include <stdlib.h> | ||
#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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I suggest this gets trimmed in a follow up PR |
||
|
||
#include "bundle.h" | ||
|
||
|
@@ -35,8 +39,11 @@ | |
} \ | ||
} while (0) | ||
|
||
const char* g_argv0 = NULL; | ||
|
||
TVM_DLL void* tvm_runtime_create(const char* json_data, const char* params_data, | ||
const uint64_t params_size) { | ||
const uint64_t params_size, const char* argv0) { | ||
g_argv0 = argv0; | ||
int64_t device_type = kDLCPU; | ||
int64_t device_id = 0; | ||
|
||
|
@@ -86,5 +93,30 @@ TVM_DLL void tvm_runtime_get_output(void* runtime, int32_t index, DLTensor* tens | |
|
||
void __attribute__((noreturn)) TVMPlatformAbort(int error_code) { | ||
fprintf(stderr, "TVMPlatformAbort: %d\n", error_code); | ||
void* trace[200]; | ||
int nptrs = backtrace(trace, sizeof(trace) / sizeof(void*)); | ||
fprintf(stderr, "backtrace: %d\n", nptrs); | ||
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 commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. okay, added a Makefile flag to control this. |
||
|
||
char cmd_buf[1024]; | ||
for (int i = 0; i < nptrs; i++) { | ||
Dl_info info; | ||
if (dladdr(trace[i], &info)) { | ||
fprintf(stderr, "symbol %d: %s %s %p (%p)\n", i, info.dli_sname, info.dli_fname, | ||
info.dli_fbase, trace[i] - info.dli_fbase); | ||
snprintf(cmd_buf, sizeof(cmd_buf), "addr2line --exe=%s -p -i -a -f %p", g_argv0, | ||
trace[i] - info.dli_fbase); | ||
int result = system(cmd_buf); | ||
if (result < 0) { | ||
perror("invoking backtrace command"); | ||
} | ||
} else { | ||
fprintf(stderr, "symbol %d: %p (unmapped)\n", i, trace[i]); | ||
} | ||
} | ||
} | ||
exit(-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.
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.