stoa commented on a change in pull request #7742:
URL: https://github.com/apache/tvm/pull/7742#discussion_r626494235
##########
File path: src/runtime/stm32/graph_runtime.cc
##########
@@ -0,0 +1,529 @@
+/*
Review comment:
How are you, Andrew @areusch
I have reorganized the code like so:
1. Removed the `apps/stm32`
2. Removed linker script code from the `emitter.py`
3. Removed the copy of `graph_runtime` from `src/runtime/stm32`
3. Moved the `src/runtime/stm32` to `src/runtime/crt/contrib/stm32`. I have
chosen the `contrib' because it looks like other files in the directory are
sort of common implementation, while the `stm32` is code-generator specific. If
you prefer, not to use `contrib` for this - I will align the runtime and tests.
4. Moved the C part of our CI test to `tests/crt/contrib/stm32`
Now, I am looking at this comment:
> it seems like you could use the TVM C runtime's crt_backend_api.c and
implement TVMPlatformMemoryAllocate(). Curious if that would be an option for
you? btw, it would be totally fine with me if you wanted to split this file
into a separate CRT library e.g. src/runtime/crt/backend to avoid including the
rest of src/runtime/crt/common.
I have following observations:
1. Current TVM generates C code with these calls:
```
TVMAPISetLastError()
TVMGetLastError()
TVMBackendAllocWorkspace()
TVMBackendFreeWorkspace()
```
These would be good candidates for including with the `crt_backend_api.c`.
Therefore, moving the `TVMAPISetLastError()` and the `TVMGetLastError()` there
from the `crt_runtime_api.c`, and moving the `TVMBackendParallelLaunch()`,
`TVMBackendRegisterSystemLibSymbol()` out to `crt_runtime_api.c`.
This way, the `crt_backend_api.c` would carry a minimal support required by
the C target code generator.
2. Currently, the `TVMBackendAllocWorkspace` does some error handling. I
would propose to leave the error handling to the caller, while the
`TVMBackendAllocWorkspace` would just return `NULL` in case of error.
The reason is that the error handling and logging are platform-specific and
even application-specific - eg. `malloc` does not do error handling, for
example.
Handling this in the `TVMBackendAllocWorkspace` requires implementing the
`TVMLogf` and the `TVMPlatformAbort`, which we will not be able to test. For
example, in our `stm32` TVM test, I would have to implement the `x86` specific
error handling, while for the Disco board I would set it to NOOP since the
applications handles errors in its own way.
3. If we were to reduce the `crt_backend_api.c` to its minimal form as I
have described above, perhaps it makes sense to have the application implement
these few methods directly, i.e. no need to include the
`tvm/crt/crt_backend_api.c` implementation, only the `crt_backend_api.h` header
as interface ?
What do you think ?
Do not review the `tests/crt/contrib/stm32` yet - I will align it and clean
after I have your opinion on the above.
cheers
Arthur
--
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]
