New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TF Lite kernel tests cross-compilation enablement using CMake #52110
TF Lite kernel tests cross-compilation enablement using CMake #52110
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution
@@ -41,18 +41,37 @@ build_flatbuffers( | |||
"" | |||
) | |||
|
|||
set(DELEGATE_PROVIDERS_SUPP | |||
${TFLITE_SOURCE_DIR}/nnapi/sl/SupportLibrary.cc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plz alphabetize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
set(DELEGATE_PROVIDERS | ||
${DELEGATE_PROVIDERS_SUPP} | ||
${TFLITE_SOURCE_DIR}/tools/delegates/gpu_delegate_provider.cc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -29,6 +29,8 @@ constexpr char KernelTestDelegateProviders::kUseSimpleAllocator[]; | |||
|
|||
KernelTestDelegateProviders::KernelTestDelegateProviders() | |||
: delegate_list_util_(¶ms_) { | |||
TFLITE_TOOLS_CHECK(delegate_list_util_.GetCount() > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's happening if we don't have this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no delegate available, calling delegate_list_util.AppendCmdlineFlags(&flags) (line 46) appends no command line flags. Since at least --help flag is expected to be available (line 49), the condition returns false and an error is reported (specifically: "help was not found", as implemented in tensorflow/lite/tools/tool_params.cc:53). So this check aims to provide the user with more instructive/helpful error message, should this happen again in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out this change breaks many tests. You'd better revert it.
bazel test tensorflow/lite/kernels:optional_tensor_test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check has been reverted in the latest commits.
to build the flatc compiler with CMake in advance using the host toolchain: | ||
|
||
```sh | ||
cmake ../tensorflow_src/tensorflow/lite/tools/cmake/native_tools/flatbuffers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we use a separate directory for native_tools and tflite build? I think we'd better mention it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation has been updated to make this more obvious.
|
||
```sh | ||
cmake -DCMAKE_INSTALL_PREFIX=<native_tools_dir> ../tensorflow_src/tensorflow/lite/tools/cmake/native_tools/flatbuffers | ||
cmake --build . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we need to run install command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, running cmake --build . installs the flatc automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
##### Specifics of kernel (unit) tests cross-compilation | ||
|
||
Cross-compilation of the unit tests requires flatc compiler for the host architecture. | ||
For this purpose, there is a CMakeLists located in _tensorflow/lite/tools/cmake/native_tools/flatbuffers_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "xxx" valid markdown format?
https://www.markdownguide.org/basic-syntax/
I think it's better to use xxx
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True - updated in the latest commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plz fix failing tests.
# purposes the FLATC_INSTALL_PREFIX is already in cache and is just used in this module. | ||
# In case of standard flatbuffers build (as a dependency) the variable needs to be set. | ||
if(NOT DEFINED FLATC_INSTALL_PREFIX) | ||
set(FLATC_INSTALL_PREFIX <INSTALL_DIR> CACHE PATH "Flatc installation directory") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a space at the ending. Plz remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space removed in the latest commit.
@@ -29,6 +29,8 @@ constexpr char KernelTestDelegateProviders::kUseSimpleAllocator[]; | |||
|
|||
KernelTestDelegateProviders::KernelTestDelegateProviders() | |||
: delegate_list_util_(¶ms_) { | |||
TFLITE_TOOLS_CHECK(delegate_list_util_.GetCount() > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out this change breaks many tests. You'd better revert it.
bazel test tensorflow/lite/kernels:optional_tensor_test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR implements the enablement of the TF Lite kernel tests CMake cross-compilation using the natively built flatc compiler (for details please see the updated build_cmake.md file).
As it currently stands in the master branch, the native CMake compilation produces malformed TF Lite kernel tests. Therefore, for testing purposes, this PR also encompasses changes implemented by the currently reviewed PR which fixes it.