areusch edited a comment on pull request #5921: URL: https://github.com/apache/incubator-tvm/pull/5921#issuecomment-656878983
@liangfu thanks for taking a look. I looked more into your error and it looks like I overlooked testing bundle_dynamic most recently. that is fixable, however... I did some more investigation and there are a couple of problems: 1. I believe the performance regression is related to switching to `c` compiler, based on some profiling. 2. with the CRT changes proposed in this PR, we can't use `--system-lib` because the global CRT memory manager needs to be initialized before functions can be registered, and the current `--system-lib` approach registers functions in static initialization. especially for targeting embedded bare-metal systems, I think it would be wise to allow a way to do arbitrary initialization before requiring use of TVM components. 3. I tried using just plain `llvm` to avoid/prove the performance regression; however, there isn't a way to create a statically-linked non-system-lib right now (i.e. see [codegen init here](https://github.com/apache/incubator-tvm/blob/master/src/target/llvm/llvm_module.cc#L209)). i'd like to fix this, but I don't want to make this PR any larger than it is right now. 4. finally, all of this is conspiring to make the `bundle_deploy` make situation more complex. bundle_deploy produces 3 targets: C main statically-linked against CRT runtime, c++ main() dynamically loading pure-c CRT, and c++ main dynamically loading c++ Runtime. the last one requires --system-lib, while the other two can't have it. so we'll need to produce a model/test_model that targets the c++ runtime to keep the last target alive. in a future PR, I plan to add an additional flag to the target string --runtime=<crt|c++>. this flag would allow us to produce a statically-linked llvm module compatible with the C runtime. would it be okay to do the following til then: 1. use `c` module for all CRT bundle_deploy targets 2. build 2 copies of test_model and model: one for CRT and one for c++ runtime 3. for now, CRT targets will need to take a performance regression until we can re-enable the llvm module approach according to my plan above. maybe @tqchen has more thoughts here? also, it would be helpful if we could add this app to the CI somehow. any thoughts on that? ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
