stoa commented on pull request #7742:
URL: https://github.com/apache/tvm/pull/7742#issuecomment-855391490
@areusch @tqchen
Hello,
Let then sort out the review feedback in the order of importance:
> I'd like to avoid merging an alternative implementation of
crt_backend_api.c if possible. Could we merge the other PR first and then use
it? I'd like to avoid replacing layers below TVM's operator libraries where
possible.
I can propose the following:
- Do we agree on the following "software stack" ?:
inference_application[c,so]
|
c_backend_api.[h,c]
| |
c_runtime_api[h,c' |
TVM RUNTIME AOT
| |
platform-specific[h,c]
- Do we agree that, in the AOT path, we do not want to include/compile the
c_runtime_api.[h,c] ?
- 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 ?
In this case, I can move the 'g_last_error' (TVMGetSetLastError()) from the
c_runtime_api[h,c] to the 'c_backend_api.[h,c]' and remove the duplicate
'g_last_error' functionality from the platform-specific implementation,
'runtime.c'.
The 'TVMAPIErrorf()' from the 'c_runtime_api.c' will have to be changed not
to use the 'g_last_error', I have an impression that it is already pretty much
the case.
> Platform-specific defines in 'ai_platform.h': I need to verify and will
get back on this point quickly.
I am noticing that some of similar defines exist in 'c_runtime_api.h' and
this is used pretty extensively throughout the TVM.
However, if we agree that the 'c_runtime-api[h,c]' is not to be used within
the AOT path, we cannot use it.
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 there are still some open items on the C code (e.g. the
.clang-format should get removed and reformatted; may need to see if files in
src/runtime/contrib/stm32 should be somehow placed under src/runtime/crt,
src/runtime/micro, or src/micro, since they aren't intended to be compiled with
the C++ runtime), but not sure if you're waiting to address the ai_platform.h
question there.
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.
> I guess my concern with keeping the headers in place is that TVM code is
supposed to be fairly uniform in style.
I will remove the headers.
> 'test_mnist_quant_fp': just wondering if the delete here was intentional?
Yes, intentional. We do not have a possibility to host these test-cases for
downloading and they cannot be dowloaded from elsewhere.
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.
--
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:
us...@infra.apache.org
