[GitHub] [tvm] areusch commented on pull request #7742: Contributing the STM32 port

[GitBox <gi...@apache.org>]

areusch commented on pull request #7742:
URL: https://github.com/apache/tvm/pull/7742#issuecomment-856072634


   @stoa thanks for your detailed reply. i've tried to provide answers to all of your questions--let me know if a higher-bandwidth discussion would be helpful to align here (we can use the discord and document here)
   
   > Do we agree on the following "software stack" ?:
   i'd draw the software stack like so:
   ```
         deployed inference_application       generic app for host-driven inference/autotvm
                   |                                                           |
          firmware-facing API                                                  |
                   |                                                           |
                  AOT [c]          graph executor ----- c_runtime_api -- utvm_rpc_server
                      |                    |                 |
                     compiled operators [c,so]               |
                                     |                       |
                         c_backend_api.[h,c]  <--------------+
                                 |
                       platform-specific[platform.h, platform-specific implementation location]
   ```
   
   Does this make sense? The c_backend_api is mainly a glue layer between the API exposed to operator functions and the platform-specific API, but it allows them to change independently of one another.
   
   one note I'll make: right now "deployed inference application" as demonstrated/tested against Zephyr would use graph_executor and c_runtime_api. This is just due to the fact that we haven't finished developing the firmware-facing AOT interface yet.
   
   > Do we agree that, in the AOT path, we do not want to include/compile the c_runtime_api.[h,c] ?
   
   yeah definitely. i hope the above diagram clarifies my position on that, and apologies for any confusion. Stuff is changing fast right now.
   
   > Do we agree that the 'g_last_error' (TVMGetSetLastError()) is set by the running inference application and not by the lower runtime layers: c_runtime_api|platform_specific ?
   
   i think right now anything above c_backend_api could set that. c_runtime_api should be allowed to consume c_backend_api imo.
   
   > I can propose to create some sort of 'c_runtime_defs.h' that would include platform-specific defines for:
   >
   >AI_API_ENTRY = TVM_DLL
   >AI_API_ALIGNED
   >AI_API_WEAK = TVM_WEAK
   >etc.
   >The platform-specific implementation can include this file then instead of the 'c_runtime_api.h'.
   >Still the question of testing all different defines remain - but this is already the case with the 'c_runtime_api.h'.
   
   i think one question is whether we want to promote the `AI_API_` prefix to be the TVM standard prefix. if so, this makes sense to me. however, i think such a proposal would need to be explicitly in an RFC on the discuss. I think we'd like to ultimately wind up with something compatible either way, but perhaps it makes sense to confine this `c_runtime_defs.h` to the STM32 code for now and promote it once the API settles. One thing is that since AI_ prefix is sort of used by STM32 code, using it here would mean that we may need to be careful about stepping on you--whereas keeping with `tvm_` prefix should be more safe.
   
   > Moving to a different code location is not an issue, please let us know which directory is most suitable.
   > And I also see that there is no reason for us to not reformat the C code - OK.
   
   OK--here's what i think:
   1. if it stays in `src/runtime/contrib/stm32`, it needs to get formatted to the TVM guidelines
   2. if it lives outside of `src`, particularly in e.g. `apps/microtvm/stm32`, i don't think the formatting needs to follow TVM guidelines. 
   
   i've been working on the project-generator more this past week and will post it this week. in that branch, i've moved all of the zephyr code into [`apps/microtvm/zephyr/demo_runtime`](https://github.com/areusch/incubator-tvm/tree/project-generator/apps/microtvm/zephyr/demo_runtime)--in particular, the "emitter" is in `microtvm_api_server.py`. i'd ideally like to keep all the platform-specific stuff underneath `apps/microtvm/<platform>`, which allows TVM to interface a single target (the Model Library Format + microtvm API server) but continue to target multiple platforms. in exchange, code in that directory doesn't need to necessarily follow all of TVM's C formatting standards (e.g. there is iOS code in some of those directories).
   
   i realize we had discussed this approach for stm32 before, but things weren't quite ready; plus you guys wanted to provide an improved API. we've discussed moving in the direction of improving microTVM's API such that the STM32 API can be provided by wrapping the microTVM API. when we reach that point, i'd like to move the STM32 code underneath `apps/microtvm/stm32` and use the project API (but, we may need to add to the API to allow generating libraries rather than whole projects, since the project generation for STM32 is handled by X-Cube tooling). 
   
   if you want to locate your code in `apps/microtvm/stm32` now and avoid reformatting, even if it doesn't use project generator, that would also be fine with me--let me know if this makes sense or not to you guys.
   
   > Yes, intentional. We do not have a possibility to host these test-cases for downloading and they cannot be dowloaded from elsewhere.
   
   If you want to push a PR to https://github.com/tlc-pack/web-data (see `testdata/microTVM`), we could host them there.
   
   > Resuming: If we agree on re-organizing the 'c_runtime-api' as outlined above, I will align this PR and we will not need a separate PR in this case.
   
   yeah that sounds fine to me. ideally it'd be a separate PR just to keep things neat, but it's not a big change so if it keeps things operationally simpler for you, feel free to include it here.


-- 
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:
users@infra.apache.org