stoa commented on pull request #7742: URL: https://github.com/apache/tvm/pull/7742#issuecomment-830804983
Hello, Andrew @areusch > 1. I think all the code currently in apps/stm32 should live outside the TVM tree. This is acceptable - I can remove it from the TVM. > 2. I think we should move the code in src/runtime/stm32 plus the code in tests/micro/stm32/src/ into apps/micro/stm32. Organizationally, I'd prefer to keep src/runtime dedicated to the TVM runtime logic. Since, as discussed on the RFC, the medium-term direction will be to move the TVM runtime in the direction of the one you proposed, I would rather consider this an "app" than an official implementation. As we move the TVM runtime forward, presumably this will eventually resemble a small shim or would disappear if the TVM runtime APIs are moved to match these (I would sort of expect there to be some minor differences in the long run between TVM and the cross-toolchain STM X-Cube AI APIs). If we do this, it will complicate our own code maintenance and we may overtime diverge from TVM. This is why: 1. The src/runtime and src/runtime/contrib already contain a number of target-specific runtime implementations, while the apps contains really the application/integration examples. 2. As I have mentioned in the RFC, we insist on separating the Application code from the Tools. The idea for us is to have our code generator + runtime part of the TVM distributed with the TVM. We want to avoid to refer to tvm/apps for accessing the runtime layer. If this were to be checked into the apps directory, we will certainly have to structure it differently with our tools offering, which will lead to needing to maintain two repositories - not what we need. **For us it is an official TVM for STM32 implementation rather than an app**. 3. I sort of disagree that the slim layer is bound to completely disappear. On the contrary, there will most certainly be a need to such a slim adaptation layer between the micro TVM API and any particular target system, for example for the memory allocation part. Isn't it better that these slim layers live in the src/runtime rather then in apps ? Perhaps we can figure out some middle ground here: - We could move the src/runtime/stm32 to the src/runtime/contrib/stm32, or - We could rename it into src/runtime/(contrib)/x-cube to resemble more coreml, rocm, cuda, etc. - We do not have any particular position on the tests/ directory - I will integrate our test anyway you deem appropriate, can go to the apps/stm32. However, I did not understand: are you proposing to separate the src part of the test from the rest ? > I'd suggest we don't output device-specific things (e.g. linker scripts) in `python/tvm/contrib/stm32/emitter.py.` I will remove this. The following is already taken care of: > everything in the TVM tree needs to get apache-licensed, so by checking in that code we need to be sure that's cool with you guys The TVM license header is already added to all code. > anything in the TVM tree will need a defined path to be tested (ideally one that's automated) and any maintenance will need to go through our code review process This is understood. This is why we want our runtime layer to be integrated as a "real" runtime with the TVM - it will be the only place it lives in and is maintained: it is TVM specific.. What do you think ? -- 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]
