[test-suite] Fix support for user mode emulation when using cmake/lit.

[kristof.beyls]

Summary

When using user mode emulation, i.e. cross-compiling programs for a
different target and running them on a host under qemu user mode
emulation, timeit and fpcmp should have host versions, not target
versions.

Running under user mode emulation has been broken for a while,
presumably since https://reviews.llvm.org/rT341257

I first tried an alternative approach where fpcmp would be run under qemu user mode emulation too.
That in itself worked, but if going for that approach, for orthogonality reasons, we probably should also run the other helper programs as if they were running on the target, i.e. also under qemu user mode emulation.
I ran into issues with running timeit under qemu user mode emulation and also running RunSafely.sh under user mode emulation doesn't seem trivial.

In the end, it seemed better to me to explicitly add a cmake option to mark that we're running under qemu user mode emulation, and in that mode, only aim to run the test/benchmark under qemu user mode emulation, rather than also all the helper programs (such as fpcmp, timeit, RunSafely.sh) under it (which is what would be needed if we just kept on using only the RUN_UNDER option for qemu user mode emulation.

[kristof.beyls]

I haven't created any tests for this yet - I thought I'd first try and get feedback on whether this approach seems right before I spent time learning about how to get the test-suite unit/regression tests set up.

[lenary]

We have also been running into issues trying to run the test suite under qemu (for risc-v).

This patch seems (at first glance) to solve some of these issues. I think it's good that there is now an explicit "TEST_SUITE_USER_MODE_EMULATION" flag, which can be used in a variety of parts of the codebase (including the lit configuration) where required, and declares intent a little better than just RUN_UNDER alone.

I'm happy to report that our configuration of the suite, when we apply this patch, is now using the correct timeit and fpcmp binaries.

I would like you to fix the CMake issue I note below.

CMakeLists.txt

230

Currently this patch only copes with using 1 to denote True, which is less than CMake normally accepts ("On" is a good, explicit value that CMake allows, in addition to "True").

However, there is a hack that this CMake configuration uses for TEST_SUITE_PROFILE_GENERATE which we can replicate inside this if: set the value to an explicitly python-like string "True" (in this branch) and "False" (in an else branch). Then you can just directly set config.user_mode_emulation = @TEST_SUITE_USER_MODE_EMULATION@ in lit.site.cfg.in, and the configuration should be less brittle to exactly which boolean value that someone chooses at the command line for CMake.

In the future, it would probably make sense to have a python function for lit.site.cfg.in which can parse any cmake-valid boolean from a string into the correct python boolean, which feels like a nicer solution, but can come along later.

[kristof.beyls]

Thanks for the feedback Sam!

I've followed your suggestion to set the value of TEST_SUITE_USER_MODE_EMULATION in a similar way as TEST_SUITE_PROFILE_GENERATE. I agree that's a bit cleaner.

I also looked further into creating a test in the test-suite's regression tests (in litsupport-tests) for this new functionality. However, the framework that exists there is really only for the lit-part of the running tests, not the cmake part of configuring.
Since this patch is fully on the cmake side, I don't think there is any way to add a test in litsupport-tests for this. A testing framework would need to be added to test the cmake logic of the test-suite if we'd want to test this. I've decided not to try and do that for now.

I wonder what the other issues are you're seeing after applying this patch?
I also wonder if you've also tried the corresponding lnt patch at D61598 that makes it possible to use this new functionality from lnt runtest test-suite?

[lenary]

In D61597#1577701, @kristof.beyls wrote:

I wonder what the other issues are you're seeing after applying this patch?
I also wonder if you've also tried the corresponding lnt patch at D61598 that makes it possible to use this new functionality from lnt runtest test-suite?

I don't think I've run into any further issues (that are the test-suite's fault) since applying your patch.

Thanks for pointing me to that patch for LNT. I will see if it works for me. Until yesterday I was just invoking cmake directly, not via LNT, but now I need LNT's CSV-making powers.

I'd be happy for this to land, but I don't know what else needs to be checked before it can.

[kristof.beyls]

In D61597#1577730, @lenary wrote:

In D61597#1577701, @kristof.beyls wrote:

I wonder what the other issues are you're seeing after applying this patch?
I also wonder if you've also tried the corresponding lnt patch at D61598 that makes it possible to use this new functionality from lnt runtest test-suite?

I don't think I've run into any further issues (that are the test-suite's fault) since applying your patch.

Thanks for pointing me to that patch for LNT. I will see if it works for me. Until yesterday I was just invoking cmake directly, not via LNT, but now I need LNT's CSV-making powers.

FWIW, I also have a downstream patch for LNT to use comma as the separator, rather than semi-colon. I find that results in less clicking being needed when opening the CSV file in Excel. Happy to upstream that too if you also find out that commas, rather than semi-colons, as a field separator in CSV files works better.

I'd be happy for this to land, but I don't know what else needs to be checked before it can.

I don't think anything else needs to be checked. It'd be great if the test-suite itself had a regression testing system as good as e.g. llvm or clang so that with regression tests even more confidence could be had in changes made. But that is an orthogonal issue.

Could you on this review mark that you've approved this patch ("Accept Revision")? Maybe you'll need to first make yourself a reviewer to be able to do that - I'm not sure.

[lenary]

In D61597#1577801, @kristof.beyls wrote:

FWIW, I also have a downstream patch for LNT to use comma as the separator, rather than semi-colon. I find that results in less clicking being needed when opening the CSV file in Excel. Happy to upstream that too if you also find out that commas, rather than semi-colons, as a field separator in CSV files works better.

I don't mind so much about the CSV being semicolon-separated, I can cope with the extra clicks, and I don't want to break anyone else's systems. That said, happy for you to post the patch so further discussion can happen.

Could you on this review mark that you've approved this patch ("Accept Revision")? Maybe you'll need to first make yourself a reviewer to be able to do that - I'm not sure.

Sure, done!

CMakeLists.txt

COMMAND ${PROJECT_SOURCE_DIR}/utils/rsync.sh
                 ${TEST_SUITE_REMOTE_HOST} ${PROJECT_BINARY_DIR}
     USES_TERMINAL
   )
 endif()
 
 # Run Under configuration for RunSafely.sh (will be set in lit.site.cfg)
 set(TEST_SUITE_RUN_UNDER "" CACHE STRING "RunSafely.sh run-under (-u) parameter")
 
+# User mode emulation configuration (e.g. running under qemu)
+# (will be set in lit.site.cfg)
+set(TEST_SUITE_USER_MODE_EMULATION NO CACHE BOOL
+    "RUN_UNDER is used to run tests under emulation.")
+
 # run type/benchmark size configuration (mostly for SPEC at the moment)
 set(TEST_SUITE_RUN_TYPE "train" CACHE STRING
     "Type of benchmark inputs (may be test,train or ref)")
 
 get_filename_component(CMAKE_C_COMPILER_DIRECTORY ${CMAKE_C_COMPILER} DIRECTORY)
 
 option(TEST_SUITE_COLLECT_CODE_SIZE "Measure code size of binaries" ON)
 if(TEST_SUITE_COLLECT_CODE_SIZE)

 
 find_program(TEST_SUITE_LIT NAMES "lit" "llvm-lit")
 set(TEST_SUITE_LIT_FLAGS "-sv" CACHE STRING "Flags used when running lit")
 mark_as_advanced(TEST_SUITE_LIT TEST_SUITE_LIT_FLAGS)
 mark_as_advanced(TEST_SUITE_LIT)
 
 add_subdirectory(tools)
 # Shortcut for the path to the fpcmp executable
+set(FPCMP ${CMAKE_BINARY_DIR}/tools/fpcmp-target)
+if (TEST_SUITE_USER_MODE_EMULATION)
   set(FPCMP ${CMAKE_BINARY_DIR}/tools/fpcmp)
+endif()
 
 option(TEST_SUITE_COLLECT_COMPILE_TIME
        "Measure compile time by wrapping compiler invocations in timeit" ON)
 if(TEST_SUITE_COLLECT_COMPILE_TIME)
   set(CMAKE_C_COMPILE_OBJECT "${CMAKE_BINARY_DIR}/tools/timeit --summary <OBJECT>.time ${CMAKE_C_COMPILE_OBJECT}")
   set(CMAKE_CXX_COMPILE_OBJECT "${CMAKE_BINARY_DIR}/tools/timeit --summary <OBJECT>.time ${CMAKE_CXX_COMPILE_OBJECT}")
   set(CMAKE_C_LINK_EXECUTABLE "${CMAKE_BINARY_DIR}/tools/timeit --summary <TARGET>.link.time ${CMAKE_C_LINK_EXECUTABLE}")
   set(CMAKE_CXX_LINK_EXECUTABLE "${CMAKE_BINARY_DIR}/tools/timeit --summary <TARGET>.link.time ${CMAKE_CXX_LINK_EXECUTABLE}")

cmake/modules/TestSuite.cmake

 # Add dependencies required for compiletime measurements to a target. You
 # usually do not need to call this directly when using `llvm_test_executable`
 # or `llvm_test_library`.
 function(test_suite_add_build_dependencies target)
   add_dependencies(${target}
     build-HashProgramOutput.sh
     build-timeit
     build-timeit-target
-    fpcmp
+    build-fpcmp
+    build-fpcmp-target
   )
 endfunction()
 
 # Internal function that transforms a list of flags to a string and appends
 # it to a given property of a target.
 function(append_target_flags propertyname target)
   if(NOT "${ARGN}" STREQUAL "")
     get_target_property(old_flags ${target} ${propertyname})

lit.site.cfg.in

 import sys
 
 config.test_source_root = "@CMAKE_BINARY_DIR@"
 config.test_exec_root = "@CMAKE_BINARY_DIR@"
 config.remote_client = "@TEST_SUITE_REMOTE_CLIENT@"
 config.remote_host = "@TEST_SUITE_REMOTE_HOST@"
 config.run_under = "@TEST_SUITE_RUN_UNDER@"
+config.user_mode_emulation = "@TEST_SUITE_USER_MODE_EMULATION@" == "1"
 config.strip_tool = "@CMAKE_STRIP@"
 config.profile_generate = @TEST_SUITE_PROFILE_GENERATE@
 config.llvm_profdata = "@TEST_SUITE_LLVM_PROFDATA@"
 config.llvm_size = "@TEST_SUITE_LLVM_SIZE@"
 config.test_modules = "@LIT_MODULES@".split(';')
 
 lit_config.load_config(config, "@CMAKE_SOURCE_DIR@/lit.cfg")

litsupport/modules/timeit.py

 from litsupport import shellcommand
 from litsupport import testplan
 import os
 import re
 
 
 def _mutateCommandLine(context, commandline):
     timefile = context.tmpBase + ".time"
     config = context.config
     cmd = shellcommand.parse(commandline)
 
-    timeit = "%s/tools/timeit-target" % config.test_source_root
+    if config.user_mode_emulation:
+        # user_mode_emulation should be true if tests are being run via
+        # user-mode emulation (e.g. Qemu) and thus the host version of timeit
+        # should be used.
+        timeit_name = "timeit"
+    else:
+        timeit_name = "timeit-target"
+    timeit = "%s/tools/%s" % (config.test_source_root, timeit_name)
     args = ["--limit-core", "0"]
     args += ["--limit-cpu", "7200"]
     args += ["--timeout", "7200"]
     args += ["--limit-file-size", "104857600"]
     args += ["--limit-rss-size", "838860800"]
     workdir = cmd.workdir
     if not config.traditional_output:
         stdout = cmd.stdout

tools/CMakeLists.txt

 # Tools for compiling and running the benchmarks.
 #
 # Note: Tools used while running the benchmark should be (cross-)compiled
 # normally while tools used for building the benchmark need to be built for
 # the host system (even when cross-compiling the benchmark) with
 # `llvm_add_host_executable`.
 
 include(Host)
 
-add_executable(fpcmp fpcmp.c)
+add_executable(fpcmp-target ${CMAKE_CURRENT_SOURCE_DIR}/fpcmp.c)
+add_executable(build-fpcmp-target ALIAS fpcmp-target)
+llvm_add_host_executable(build-fpcmp fpcmp fpcmp.c)
 
 add_custom_command(
   OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/HashProgramOutput.sh
   COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_SOURCE_DIR}/../HashProgramOutput.sh ${CMAKE_CURRENT_BINARY_DIR}/HashProgramOutput.sh
   COMMAND chmod +x ${CMAKE_CURRENT_BINARY_DIR}/HashProgramOutput.sh
   DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/../HashProgramOutput.sh
 )
 add_custom_target(build-HashProgramOutput.sh DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/HashProgramOutput.sh)