[Hexagon] Add support for on-device unit testing using gtest #11145

adstraw · 2022-04-27T17:33:10Z

Adds Hexagon on-device unit testing support by linking the tvm runtime with gtest. Also moves HexagonBuffer unit tests from their original home as part of the cpptest executable to their new home as a Hexagon on-device unit test. This is a Hexagon specific and proof-of-concept implementation for this discussion topic.

cc @mehrdadh

mehrdadh · 2022-04-28T15:29:02Z

Is there a reason that you put test files under src/runtime/hexagon/tests/ instead of tests/cpp/...?

CMakeLists.txt

csullivan · 2022-04-28T18:47:16Z

src/runtime/hexagon/tests/run_all_tests.cc

+
+  // initialize gtest with arguments and run
+  ::testing::InitGoogleTest(&argc, argv.data());
+  *rv = RUN_ALL_TESTS();


Neat! looks like gtest supports capturing the stdout to a string that we can return in the ret value across the FFI,

testing::internal::CaptureStdout(); // Run tests // ... std::string output = testing::internal::GetCapturedStdout();

Unfortunately, it looks like stdout redirection is one of the disabled features in the Hexagon SDK gtest version.

How so? in that case can we target a generic gtest and then modify to support for Hexagon as we have discussed potentially doing?

My understanding is that one must set GTEST_HAS_STREAM_REDIRECTION=0 when compiling gtest for Hexagon which disables the CaptureStdout function. I believe this would be true even if we used a "stock" gtest version as opposed to the gtest version in the Hexagon SDK.

Darn. @kparzysz-quic any idea why this is disabled for Hexagon? Is there an alternative for us to capture stdout on Hexagon?

I don't know. What happens if we set GTEST_HAS_STREAM_REDIRECTION=1?

GTest stream redirection relies on dup2(), which does not exist under QuRT.

This may be useful: https://google.github.io/googletest/advanced.html#extending-googletest-by-handling-test-events

adstraw · 2022-04-29T19:00:33Z

Is there a reason that you put test files under src/runtime/hexagon/tests/ instead of tests/cpp/...?

Short answer: We need a separate directory from which to include runtime tests.

Long answer ...

All files in tests/cpp/... are recursively globbed and added to the cpptest executable. This forces all code tested by cpptest to have an x86 implementation. For runtime code, it may or may not be possible to have an x86 implementation.

The HexagonBuffer class, for example, was able to have both a Hexagon (VTMC allocation via the SDK) and x86 (malloc) implementation and hence was testable using cpptest on x86. However, this added some complication to the build system which is removed in this PR along with the x86 implementation for HexagonBuffer. HexagonBuffer is now implemented and tested on Hexagon.

Other Hexagon classes e.g. for thread management coming soon to TVM will not be so easily implemented on x86 which is part of the motivation for this PR.

csullivan · 2022-04-29T21:03:12Z

I think @areusch may argue this is a reasonable example of why we should have test files co-located with source implementations. That said, for now we could consider changing the cpptest globbing to exclude the runtime subdirectory with the assumption that runtime specific tests will need to be globbed by a tvm_runtime compatible gtest impl.

adstraw · 2022-05-02T17:00:32Z

@kparzysz-quic please review

adstraw · 2022-05-02T18:29:06Z

for now we could consider changing the cpptest globbing to exclude the runtime subdirectory with the assumption that runtime specific tests will need to be globbed by a tvm_runtime compatible gtest impl.

I don't mind the current src/runtime/hexagon/tests file location for Hexagon tests but I am also open to changing it based on feedback from @areusch @mehrdadh @csullivan. Let me know what you think.

CMakeLists.txt

areusch · 2022-05-02T19:48:01Z

@adstraw until such a future time as we colocate all the tests with the source files, i think it'd be great to keep them underneath tests/ so we don't vary too much impl-to-impl. i'm okay with creating further directories underneath tests/cpptest as needed for this.

adstraw · 2022-05-02T23:28:22Z

Given that cpptest recursively globs all files under tests/cpp I decided to make a new folder named tests/cpp-runtime with subdirectory for Hexagon. I am hoping this will satisfy the concerns about test location.

kparzysz-quic · 2022-05-03T20:27:54Z

cmake/modules/Hexagon.cmake

+if(BUILD_FOR_HEXAGON OR USE_HEXAGON_RPC)
+  # Common sources for TVM runtime with Hexagon support
+  file_glob_append(RUNTIME_HEXAGON_SRCS
+    "${TVMRT_SOURCE_DIR}/hexagon/*.cc"
+  )
+else()
+  file_glob_append(RUNTIME_HEXAGON_SRCS
+    "${TVMRT_SOURCE_DIR}/hexagon/hexagon_module.cc"
+  )
+endif()


~~This part disables Hexagon runtime when building for x86. Please revert this.~~

Nevermind. It was an error on my side. Sorry for the noise.

…11145) * link gtest to tvm runtime * first test running! * HexagonBuffer tests running in sim * move to new tests directory * use USE_HEXAGON_SDK * add python frontend for Hexagon unit tests * clean up after rebase * isolate cmake changes to Hexagon * add gtest init with arguments * add hexagon sources only if building for Hexagon; remove workaround * format & lint * fix Hexagon build error * remove x86 implementation and win32 code * check if hexagon gtest path exists before linking * make USE_HEXAGON_GTEST an optional cmake param * turn on Hexagon gtest in Hexagon CI * Hexagon unit tests should fail if run without proper gtest linkage * add tvm option; move Hexagon tests to test/cpp-runtime/hexagon * add libinfo * trigger ci

adstraw added 12 commits April 22, 2022 08:35

link gtest to tvm runtime

cd11d02

first test running!

31fc623

HexagonBuffer tests running in sim

79c2110

move to new tests directory

8a2ff6d

use USE_HEXAGON_SDK

a85c333

add python frontend for Hexagon unit tests

b4db712

clean up after rebase

8ec34af

isolate cmake changes to Hexagon

4eb97fd

add gtest init with arguments

262dc73

add hexagon sources only if building for Hexagon; remove workaround

b6b0c4b

format & lint

273a2dd

fix Hexagon build error

8916768

remove x86 implementation and win32 code

39c2275

adstraw force-pushed the straw-link-gtest-to-tvm-runtime branch from edb2fc9 to 39c2275 Compare April 28, 2022 18:04

csullivan reviewed Apr 28, 2022

View reviewed changes

adstraw added 3 commits April 28, 2022 17:07

check if hexagon gtest path exists before linking

d1f1784

make USE_HEXAGON_GTEST an optional cmake param

32e6dd2

turn on Hexagon gtest in Hexagon CI

f12efd7

Hexagon unit tests should fail if run without proper gtest linkage

fa19d82

adstraw marked this pull request as ready for review May 2, 2022 15:43

kparzysz-quic approved these changes May 2, 2022

View reviewed changes

csullivan reviewed May 2, 2022

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

add tvm option; move Hexagon tests to test/cpp-runtime/hexagon

e6222e8

github-actions bot requested a review from mehrdadh May 2, 2022 23:22

adstraw added 2 commits May 2, 2022 16:46

add libinfo

816f3be

trigger ci

3946e23

csullivan merged commit 0fb155c into apache:main May 3, 2022

kparzysz-quic reviewed May 3, 2022

View reviewed changes

adstraw deleted the straw-link-gtest-to-tvm-runtime branch May 10, 2022 15:42

driazati mentioned this pull request Jul 14, 2022

TVM v0.9.0.rc0 Release Candidate Notes #12102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Hexagon] Add support for on-device unit testing using gtest #11145

[Hexagon] Add support for on-device unit testing using gtest #11145

adstraw commented Apr 27, 2022 •

edited by github-actions bot

Loading

mehrdadh commented Apr 28, 2022 •

edited

Loading

csullivan Apr 28, 2022

adstraw Apr 29, 2022

csullivan Apr 29, 2022

adstraw Apr 29, 2022

csullivan May 2, 2022

kparzysz-quic May 2, 2022

supersat May 2, 2022

supersat May 2, 2022

adstraw commented Apr 29, 2022 •

edited

Loading

csullivan commented Apr 29, 2022

adstraw commented May 2, 2022

adstraw commented May 2, 2022

areusch commented May 2, 2022

adstraw commented May 2, 2022

kparzysz-quic May 3, 2022 •

edited

Loading

[Hexagon] Add support for on-device unit testing using gtest #11145

[Hexagon] Add support for on-device unit testing using gtest #11145

Conversation

adstraw commented Apr 27, 2022 • edited by github-actions bot Loading

mehrdadh commented Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adstraw commented Apr 29, 2022 • edited Loading

csullivan commented Apr 29, 2022

adstraw commented May 2, 2022

adstraw commented May 2, 2022

areusch commented May 2, 2022

adstraw commented May 2, 2022

kparzysz-quic May 3, 2022 • edited Loading

Choose a reason for hiding this comment

adstraw commented Apr 27, 2022 •

edited by github-actions bot

Loading

mehrdadh commented Apr 28, 2022 •

edited

Loading

adstraw commented Apr 29, 2022 •

edited

Loading

kparzysz-quic May 3, 2022 •

edited

Loading