Reland "[benchmark] Re-enable building benchmark tests"

[DavidSpickett]

Summary

This reverts commit f6b04890c21ac634fcba6075017ec1487a85e132.

In MicroBenchmarks/libs/benchmark/CMakeLists.txt we decide
whether to enable assemble tests using function
should_enable_assembly_tests.

For example they are not enabled for non x86 platforms.

So the later dependencies should be guarded with a
check for BENCHMARK_ENABLE_ASSEMBLY_TESTS.

[mtrofin]

LGTM - thanks!

MicroBenchmarks/libs/CMakeLists.txt

17

Nit: could you indent lines 17-19? Thanks!

[DavidSpickett]

This seems logical to me but putting in review just in case you had other goals with this change. Test suite passes on AArch64 with this and I see MicroBenchmark mentions in the results so I assume it's doing the right thing.

See https://github.com/llvm/llvm-test-suite/blob/main/MicroBenchmarks/libs/benchmark/CMakeLists.txt#L52 for the place where we decide to enable asm tests.

[DavidSpickett]

MicroBenchmarks/libs/CMakeLists.txt

17

They're 2 space intended, that seems to match the other cmake files.

[DavidSpickett]

MicroBenchmarks/libs/CMakeLists.txt

17

indented*

[mtrofin]

MicroBenchmarks/libs/CMakeLists.txt

17

huh. Indeed! Sorry.

It seems that if I have the browser window squeezed to 1/2 screen, the indent isn't shown.

[azharudd]

This is failing for arm64 with iPhoneOS SDK.

https://green.lab.llvm.org/green/job/test-suite-verify-machineinstrs-aarch64-O3/9835/consoleFull

[mtrofin]

In D106155#2885088, @azharudd wrote:

This is failing for arm64 with iPhoneOS SDK.

https://green.lab.llvm.org/green/job/test-suite-verify-machineinstrs-aarch64-O3/9835/consoleFull

The first error I'm seeing is:

/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.5.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSObjCRuntime.h:492:1: error: expected unqualified-id
@class NSString, Protocol;
^
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.5.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSObjCRuntime.h:494:9: error: unknown type name 'NSString'
typedef NSString * NSExceptionName NS_EXTENSIBLE_STRING_ENUM;

which I have trouble to see how is related to this change. Maybe I'm missing something?

[DavidSpickett]

The error comes from gtest-port.cc which does #import <Foundation/Foundation.h>. Is it possible there is some C++/Objective C mixing issue here and we need to add some a flag or build option? (though if #import works at all it must have some understanding)

[mtrofin]

In D106155#2886634, @DavidSpickett wrote:

The error comes from gtest-port.cc which does #import <Foundation/Foundation.h>. Is it possible there is some C++/Objective C mixing issue here and we need to add some a flag or build option? (though if #import works at all it must have some understanding)

Ah, I see. Can we just disable benchmark testing for iOS?

Created https://reviews.llvm.org/D106281 - I don't have a iOS build setup, though, so it'd be good if someone verified this; or we can wait for it to land?

[azharudd]

In D106155#2887338, @mtrofin wrote:

In D106155#2886634, @DavidSpickett wrote:

The error comes from gtest-port.cc which does #import <Foundation/Foundation.h>. Is it possible there is some C++/Objective C mixing issue here and we need to add some a flag or build option? (though if #import works at all it must have some understanding)

Ah, I see. Can we just disable benchmark testing for iOS?

Created https://reviews.llvm.org/D106281 - I don't have a iOS build setup, though, so it'd be good if someone verified this; or we can wait for it to land?

It should be ok to go ahead and land. We can verify it on the Green Dragon bot listed above.

MicroBenchmarks/libs/CMakeLists.txt

 # The benchmark library build system uses try_run tests to determine regex
 # Circumvent these tests and enforce std::regex
 set(CMAKE_CXX_STANDARD 11)
 set(HAVE_STD_REGEX 1)
 # When enforcing c++11 we need to disable one of the benchmark library's tests
 set(BENCHMARK_HAS_CXX03_FLAG 0)
 
-# Disable benchmark's test targets, to avoid needing to add
-# `test_suite_add_build_dependencies` for them as they evolve upstream.
-# `benchmark` is the only target we actually need.
-set(BENCHMARK_ENABLE_TESTING Off)
 add_subdirectory(benchmark)
 test_suite_add_build_dependencies(benchmark)
+test_suite_add_build_dependencies(output_test_helper)
+test_suite_add_build_dependencies(gtest)
+test_suite_add_build_dependencies(gmock)
+
+# These tests, when built with BENCHMARK_ENABLE_ASSEMBLY_TESTS is on.
+# They don't depend on the above, so we need to list them explicitly.
+if (BENCHMARK_ENABLE_ASSEMBLY_TESTS)
+  test_suite_add_build_dependencies(donotoptimize_assembly_test)
+  test_suite_add_build_dependencies(state_assembly_test)
+  test_suite_add_build_dependencies(clobber_memory_assembly_test)
+endif()