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

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

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


   @areusch @tqchen 
   
   Hello, 
   Addressed most of your review points, except of the last one below:
   
   > on the question of 'crt_backend_api.c' -- i would like to avoid having to checkin a file such as 'src/runtime/crt/contrib/stm32/runtime.c' and instead rely on crt_backend_api.c to provide a common implementation. do you want to tackle that in this PR or in a follow-on?
   
   If you look closer, the 'src/runtime/crt/contrib/stm32/runtime.c' implements the platform-specific functions, eg. the 'TVMPlatformMemoryAllocate'. I have kept an alternative implementation of these:
   - TVMBackendAllocWorkspace (not used, under ifdef)
   - TVMBackendFreeWorkspace (not used, under ifdef)
   - TVMAPISetLastError (cannot use from c_runtime_api.c until next PR implemented)
   - TVMGetLastError (cannot use from c_runtime_api.c until next PR implemented)
   I will remove them at outcome of the next PR that will clean the c_backend_api.c. For now, prefer to keep these functions here.
   
   > my comments here are around how the Python emitter is written--it would be great to ensure the style is in line with the rest of the codebase since it is going into 'python/tvm'.
   
   Aligned the code along with your comments. The only thing I'd like to keep are my headers of type:
   ```
   # ==========================================================
   #   get_input_tensor_name
   # ==========================================================
   ```
   I have moved any doc description from them into the python doc strings. However, I have a couple of scripts that rely on finding these headers. If removing them is not an absolute MUST, perhaps I can keep it. Let me know.
   
   > we need to avoid repo bloat by not adding large test data (this model is MBs). can you download this from somewhere? see download_testdata in python/tvm/contrib/download.py.
   
   Fixed that.
   
   > thinking again about the SDK - TVM split, I see a bunch of stuff here that I think makes sense in TVM, e.g. 'struct ai_network_report'_ and dependencies. then I see some other stuff e.g. this ICC-specific logic here. It seems pretty hard to test this--wondering if it's necessary to place all of this in 'ai_platform.h'?
   
   I need to verify and will get back on this point quickly.
   


-- 
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