areusch commented on a change in pull request #7742:
URL: https://github.com/apache/tvm/pull/7742#discussion_r629731022
##########
File path: src/runtime/stm32/graph_runtime.cc
##########
@@ -0,0 +1,529 @@
+/*
Review comment:
hi @stoa, sorry for the delay again. i'll take a look at the parts of
this PR outside of `tests/crt/contrib/stm32`.
> 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.
I think that makes sense to me. `crt_backend_api.c` should be able to be
largely reused across platforms.
> 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.
There's a couple concerns I have here:
1. We'd have to generate error-checking code in operator implementations,
since that is where `TVMBackendAllocWorkspace` is called from. It's hard to
generate macros such as `CHECK_EQ` there.
2. Returning NULL would obscure the error code. I added the error code for
traceability--in my experience, it's often valuable to have a trace of the
execution path that trips an error code. On embedded, it can be quite hard to
get these traces particularly in production. Therefore, having the error code
helps to debug without necessarily needing to have a debugger attached. We
could explore changing the signature of TVMBackendAllocWorkspace, but it could
break people
> 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 ?
Potentially for the memory allocation function, but I'm not so sure about
the rest. The rest are for interop with the runtime. As we add support for e.g.
accelerators or better memory planning here, I could see these implementations
becoming a bit more complex. I'd prefer to stick with the application hook
`TVMPlatformMemoryAllocate` for now, if that's the only divergence you can see
per-application.
--
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]
