-
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
[Runtime] MISRA-C compliant TVM runtime #3934
Conversation
also cc @ajtulloch Now that we have quite a few runtimes, I wonder if it makes sense to consolidate some of them, for example, i do think it makes sense to make uTVM runtime MISRA-C compliant |
This PR also contains a rewritten version of JSON parser and NDArray reader, which is relative independent and concise. I would like to propose a replacement for the |
@liangfu absolutely, this would reduce code size substantially as well I believe. I suspect that it would make sense to remove the 'uTVM runtime' and replace it with the MISRA-C one - as long as you're comfortable with code size being an important consideration in the development of the MISRA-C one? |
@liangfu @ajtulloch would be great if we can followup on this topic. e.g. try to compare the standalone utvm and misra-c runtime, and review this part of the proposed code. One related question is that is it still OK to use C++? I see most of the code are implemented in pure C, which is fine, but perhaps we could still benefit from namespace and simple class(no virtual methods). |
The major intention to implement this in pure C is to maximize portability. (Some vendors didn't offer C++ compiler for its vision processor, partly for the sake of misra-c compliance, e.g. Vision SDK from TI.) On the other hand, this reduces binary size at the cost of flexibility. |
OK, I think we could go with c API, just remember to keep naming style consistent with the current C API as in c_runtime_api.h |
Sure, I would update the naming accordingly, and add test cases as well. |
ping @liangfu please see if you are interested in continue pushing this thread |
@tqchen sure, I can continue to push this thread, although this has been suspended for a while. |
@tqchen For now, the implement reproduces apps/bundle_deploy with complete runtime implemented in pure C. Note that the build_mode.py script generates apps/bundle_deploy_c/build/bridge.c to load functions in the compiled object (model.o), so that runtime could load all the compiled functions. In addition, to keep the pure C code OOP, an additional underscore is used in member functions for structs. Please take a review. |
This looks really neat, well done @liangfu. |
@ajtulloch Thanks for the comment. In addition, I was also trying to integrate this with your implement of the standalone uTVM runtime. Hopefully, we would have a successful removal of picojson as an external dependency. |
Change-Id: I027ddff15c31fb4da0bd0e461427dce619de1f93
I have tested this on both 32-bit ARM board, and 64-bit Linux PC, they produced exactly the same result.
Nice suggestion! I've made this runs in CI. Please take anther look. |
Change-Id: Ic2e82fb3051b6c254ef32a964f976b61e3e5fe4d
Excellent, thanks for adding this to CI so quickly! I was able to reproduce the demo by typing in
|
Change-Id: Ide466a5cf21cf8bb990dcd4a1189ba17594e3c51
@liangfu The e2e code cannot be run as part of the unittest because the CPU module do not have mxnet installed. Let us create a simple function(e.g. elemwise add) that makes use of the runtime as the test function. |
ah, could we update the dockerfile to install mxnet? or will that be too wasteful in terms of CPU cycles |
In terms of testing the runtime out, using simple add one example would be sufficient. |
@liangfu would be great if you can try to update(use simple testing as in other all in one deployment that does not depend on mxnet) :) |
sure |
Change-Id: Ie0dfd0ade6be4665b4384db7d260a6c69b35010f
Change-Id: Ie7fbc16b4b0b9509918d986a841f443900813bef
@tmoreau89 @tqchen A simple test case has been added, and the testing results are successful. Please take another look. |
Thanks @liangfu @tmoreau89 @ajtulloch , this is now merged! |
Great work. @liangfu have you considered using "system lib" approach since dlopen is banned in some environments? |
@liangfu awesome work, very useful to our use-cases. |
* implement of MISRA-C compliant TVM runtime; * working on bundle_deploy_c demo * move header files into include dir * fix compatibility issues * fix compatibility issues * resolve most of the warnings and errros * implement c_backend_api * introduce bridge * working well * move to header files and bundle.c into src/runtime/crt * clean up * satisfy linter * clean up * test with the cat image * remove synset * refactoring * refactoring * refactoring * initial crt_runtime_api.c * improved compatibility with g++ * using exposed API in c_runtime_api.h * call from c_runtime_api.h * clean up * lint * merge into apps/bundle_deploy directory Change-Id: I51904db81b8589e65d107d8ca77b47452e3812b5 * make the demo runs in ci Change-Id: I2c24f8b592508833d3555311c2b24d1931f19385 * address review comments Change-Id: I027ddff15c31fb4da0bd0e461427dce619de1f93 * release Change-Id: I5ad5bb8426468aac9fc8d074e56ddea358a7fd91 * fix ci testing Change-Id: Ic2e82fb3051b6c254ef32a964f976b61e3e5fe4d * add test case for misra c runtime Change-Id: Ie0dfd0ade6be4665b4384db7d260a6c69b35010f * fread files in testing to avoid calling xxd Change-Id: Ie7fbc16b4b0b9509918d986a841f443900813bef
* implement of MISRA-C compliant TVM runtime; * working on bundle_deploy_c demo * move header files into include dir * fix compatibility issues * fix compatibility issues * resolve most of the warnings and errros * implement c_backend_api * introduce bridge * working well * move to header files and bundle.c into src/runtime/crt * clean up * satisfy linter * clean up * test with the cat image * remove synset * refactoring * refactoring * refactoring * initial crt_runtime_api.c * improved compatibility with g++ * using exposed API in c_runtime_api.h * call from c_runtime_api.h * clean up * lint * merge into apps/bundle_deploy directory Change-Id: I51904db81b8589e65d107d8ca77b47452e3812b5 * make the demo runs in ci Change-Id: I2c24f8b592508833d3555311c2b24d1931f19385 * address review comments Change-Id: I027ddff15c31fb4da0bd0e461427dce619de1f93 * release Change-Id: I5ad5bb8426468aac9fc8d074e56ddea358a7fd91 * fix ci testing Change-Id: Ic2e82fb3051b6c254ef32a964f976b61e3e5fe4d * add test case for misra c runtime Change-Id: Ie0dfd0ade6be4665b4384db7d260a6c69b35010f * fread files in testing to avoid calling xxd Change-Id: Ie7fbc16b4b0b9509918d986a841f443900813bef
May I ask some more details like which MISRA rules(MISRA-C 2004, MISRA-C++-2008, MISRA-C-2012) are fixed other than what is mentioned on the #3159 . Thanks! |
This PR implements MISRA-C compliant TVM runtime proposed in #3159 .
See timing improvements in the following table:
@tqchen @ajtulloch @nhynes Please review.