litsupport/remote: Work without shared filesystem

[MatzeB]

Summary

Change litsupport/remote code to not assume a shared filesystem anymore:

• Assumes we can run commands on the remote target and get output/returncodes back (typically ssh)
• Expects the benchmark build directory was transfered to the remove device via other means (typically rsync) before running.

Note: This whole remote support is only relevant for apples internal usage.
At least I'm not aware of anyone else using the remote litsupport module.

I am putting this up for early review feedback. In practice we cannot
commit this yet, as lnt isn't prepared to rsync things to the device
before running (for this I actually plan to get back to other patches
of mine that allow to submit to LNT without actually running the benchmarks
via LNT itself).

[Meinersbur]

Can you outline the intended workflow? My guess:

1. cmake builddir for cross-compilation
2. make/ninja
3. rsync pushes test-suite builddir to target
4. llvm-lit locale, but run every command on remote using
5. including fpmc/VALIDATE step in *.test (SPEC2017 also uses llvm_add_host_executable)
6. collect metrics from .o.time/.link.time files from local builddir and execution times using remote_read_result_file function.

Is this correct?

litsupport/modules/timeit.py

82–90

Did you consider keeping getUserTime with filename argument and extract-out the parsing step into a new function which can be called instead if there is not filename? There seems to be a bit of boilerplate added for every time the content is just a file (for collecting compile times).

tools/CMakeLists.txt

0–6

With the change below, this comment does not seem to apply anymore.

[MatzeB]

In D51080#1209934, @Meinersbur wrote:

Can you outline the intended workflow? My guess:

1. cmake builddir for cross-compilation
2. make/ninja
3. rsync pushes test-suite builddir to target
4. llvm-lit locale, but run every command on remote using
5. including fpmc/VALIDATE step in *.test (SPEC2017 also uses llvm_add_host_executable)

The VERIFY: steps in the scripts are run remotely as well. I hope that takes part of the spec2017 validation things[*]

7. collect metrics from .o.time/.link.time files from local builddir and execution times using remote_read_result_file function.

Yes, things falling out of the compilation (compiletime plugin, stats plugin) is read locally, things that get produced while running a benchmark (timeit, microbenchmark modules) use context.read_result_file() which is assigned to remote_read_result_file() when running remotely (or default_read_result_file() otherwise)

Is this correct?

Yes.

• I don't have a copy of SPEC2017 at the moment, so unfortunately I cannot actually test any of this stuff for it. We probably need some llvm_test_data() calls added to get SPEC2017 working anyway...

[Meinersbur]

In D51080#1209955, @MatzeB wrote:

In D51080#1209934, @Meinersbur wrote:

1. cmake builddir for cross-compilation
2. make/ninja
3. rsync pushes test-suite builddir to target
4. llvm-lit locale, but run every command on remote using ssh
5. including fpmc/VALIDATE step in *.test (SPEC2017 also uses llvm_add_host_executable)

The VERIFY: steps in the scripts are run remotely as well.

PREPARE: and METRIC as well?

I hope that takes part of the spec2017 validation things[*]

Yes, it should. As you mentioned, it will require the reference output on the remote as well.
It also means, we don't need llvm_add_host_executable anymore, which I consider a good thing.

[MatzeB]

In D51080#1210083, @Meinersbur wrote:

In D51080#1209955, @MatzeB wrote:

In D51080#1209934, @Meinersbur wrote:

1. cmake builddir for cross-compilation
2. make/ninja
3. rsync pushes test-suite builddir to target
4. llvm-lit locale, but run every command on remote using ssh
5. including fpmc/VALIDATE step in *.test (SPEC2017 also uses llvm_add_host_executable)

The VERIFY: steps in the scripts are run remotely as well.

PREPARE: and METRIC as well?

Yes PREPARE: is modified as well, and I just realized METRIC: should be modified too but is still TODO :)

I hope that takes part of the spec2017 validation things[*]

Yes, it should. As you mentioned, it will require the reference output on the remote as well.
It also means, we don't need llvm_add_host_executable anymore, which I consider a good thing.

ah indeed, that is a nice side effect.

[MatzeB]

• Address review comments
• Execute METRIC: lines remotely

[MatzeB]

• Fix PGO in remote setting: We have to do the merging on the host side where the compiler toolchain resides, however before we can do that we have to retrieve the profile data from device. Make the remote module add commands to retrieve the profile data.

[MatzeB]

ping

[Meinersbur]

In D51080#1216896, @MatzeB wrote:

ping

The summary mentions this is for early review. Has this changed?

I'd love a comment/documentation somewhere about the flow between local and remote.

[MatzeB]

In D51080#1217564, @Meinersbur wrote:

In D51080#1216896, @MatzeB wrote:

ping

The summary mentions this is for early review. Has this changed?

Ah yes, the first versions of this patch was not working in some corner cases like PGO. But with the latest updates I consider this patch ready.

I'd love a comment/documentation somewhere about the flow between local and remote.

I'll bring up a separate patch for the test-suite docu about running remote (which lives in the llvm repository and is confusing/out of date anyway at the moment).

In D51080#1217564, @Meinersbur wrote:

In D51080#1216896, @MatzeB wrote:

ping

The summary mentions this is for early review. Has this changed?

I'd love a comment/documentation somewhere about the flow between local and remote.

[Meinersbur]

In D51080#1217911, @MatzeB wrote:

I'll bring up a separate patch for the test-suite docu about running remote (which lives in the llvm repository and is confusing/out of date anyway at the moment).

Looking forward for the updated docs!

Overall, LGTM

[Meinersbur]

tools/CMakeLists.txt

36

Notived after looking into D51465: Isn't/should timeit be executed on the remote as well?

[MatzeB]

tools/CMakeLists.txt

36

timeit-target will be executed on the remote. However on the host we still use timeit to wrap the compiler invocations for compiletime measurements.

[Meinersbur]

tools/CMakeLists.txt

36

Thanks. I forgot about the compilation times.

CMakeLists.txt

if(NOT CMAKE_SIZEOF_VOID_P)
   message(FATAL_ERROR "CMAKE_SIZEOF_VOID_P is not defined")
 endif()
 
 # Remote configuration (will be set in lit.site.cfg)
 set(TEST_SUITE_REMOTE_CLIENT "ssh" CACHE STRING "Remote execution client")
 set(TEST_SUITE_REMOTE_HOST "" CACHE STRING "Remote execution host")
 mark_as_advanced(TEST_SUITE_REMOTE_CLIENT)
 
+add_custom_target(rsync
+  COMMAND ${PROJECT_SOURCE_DIR}/utils/rsync.sh
+              ${TEST_SUITE_REMOTE_HOST} ${PROJECT_BINARY_DIR}
+  USES_TERMINAL
+)
+
 # 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")
 
 # 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)

cmake/modules/TestSuite.cmake

 ##===- TestSuite.cmake ----------------------------------------------------===##
 #
 # Defines helper functions to build benchmarks and the corresponding .test files
 #
 ##===----------------------------------------------------------------------===##
 include(TestFile)
 include(CopyDir)
 
+set(_DEFAULT_TEST_SUITE_COPY_DATA OFF)
+if(TEST_SUITE_REMOTE_HOST)
+  set(_DEFAULT_TEST_SUITE_COPY_DATA ON)
+endif()
+option(TEST_SUITE_COPY_DATA "Always copy benchmark data to builddir"
+       ${_DEFAULT_TEST_SUITE_COPY_DATA})
+mark_as_advanced(TEST_SUITE_COPY_DATA)
+
 # Copies files and directories to be used as benchmark input data to the
 # directory of the benchmark executable.
 # Paths are interepreted relative to CMAKE_CURRENT_SOURCE_DIR by default but
 # this can be changed with the SOURCE_DIR argument.
 function(llvm_test_data target)
   cmake_parse_arguments(_LTDARGS "MUST_COPY" "SOURCE_DIR" "" ${ARGN})
   set(SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
   if(_LTDARGS_SOURCE_DIR)
     set(SOURCE_DIR ${_LTDARGS_SOURCE_DIR})
   endif()
   foreach(file ${_LTDARGS_UNPARSED_ARGUMENTS})
     set(full_path ${SOURCE_DIR}/${file})
-    if(_LTDARGS_MUST_COPY)
+    if(_LTDARGS_MUST_COPY OR TEST_SUITE_COPY_DATA)
       if(IS_DIRECTORY ${full_path})
         llvm_copy_dir(${target} $<TARGET_FILE_DIR:${target}>/${file} ${full_path})
       else()
         llvm_copy(${target} $<TARGET_FILE_DIR:${target}>/${file} ${full_path})
       endif()
     else()
       get_filename_component(file_subdir ${file} DIRECTORY)
       if(file_subdir)

 endfunction()
 
 # 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-fpcmp
     build-timeit
     build-timeit-target
+    fpcmp
   )
 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})

litsupport/modules/microbenchmark.py

 
 
 def _mutateScript(context, script):
     return testplan.mutateScript(context, script, _mutateCommandLine)
 
 
 def _collectMicrobenchmarkTime(context, microbenchfiles):
     for f in microbenchfiles:
-        with open(f) as inp:
-            lines = csv.reader(inp)
-            # First line: "name,iterations,real_time,cpu_time,time_unit..."
-            for line in lines:
-                if line[0] == 'name':
-                    continue
-                # Name for MicroBenchmark
-                name = line[0]
-                # Create Result object with PASS
-                microBenchmark = lit.Test.Result(lit.Test.PASS)
+        content = context.read_result_file(context, f)
+        lines = csv.reader(content.splitlines())
 
-                # Index 3 is cpu_time
-                exec_time_metric = lit.Test.toMetricValue(float(line[3]))
-                microBenchmark.addMetric('exec_time', exec_time_metric)
 
-                # Add Micro Result
-                context.micro_results[name] = microBenchmark
 
     # returning the number of microbenchmarks collected as a metric for the
     # base test
     return ({
         'MicroBenchmarks': lit.Test.toMetricValue(len(context.micro_results))
     })
 
 
 def mutatePlan(context, plan):
     context.microbenchfiles = []
     plan.runscript = _mutateScript(context, plan.runscript)
     plan.metric_collectors.append(
         lambda context: _collectMicrobenchmarkTime(context,
                                                    context.microbenchfiles)
     )

litsupport/modules/profilegen.py

 def _mutateScript(context, script):
     return testplan.mutateScript(context, script, _mutateCommandline)
 
 
 def mutatePlan(context, plan):
     context.profilefiles = []
     # Adjust run steps to set LLVM_PROFILE_FILE environment variable.
     plan.runscript = _mutateScript(context, plan.runscript)
+    plan.profile_files += context.profilefiles
+
     # Run profdata merge at the end
     profdatafile = context.executable + ".profdata"
     args = ['merge', '-output=%s' % profdatafile] + context.profilefiles
     mergecmd = shellcommand.ShellCommand(context.config.llvm_profdata, args)
     plan.profilescript += [mergecmd.toCommandline()]

litsupport/modules/remote.py

 """Test module to execute a benchmark through ssh on a remote device.
 This assumes all relevant directories and files are present on the remote
 device (typically shared by NFS)."""
 from litsupport import testplan
 import logging
+import os
+import subprocess
 
 
-def _mutateCommandline(context, commandline, suffix=""):
-    shfilename = context.tmpBase + suffix + ".sh"
-    shfile = open(shfilename, "w")
-    shfile.write(commandline + "\n")
-    logging.info("Created shfile '%s'", shfilename)
-    shfile.close()
-
-    config = context.config
-    remote_commandline = config.remote_client
-    remote_commandline += " %s" % config.remote_host
-    remote_commandline += " /bin/sh %s" % shfilename
-    return remote_commandline
+def _wrap_command(context, command):
+    escaped_command = command.replace("'", "'\\''")
+    return "%s %s '%s'" % (context.config.remote_client,
+                           context.config.remote_host, escaped_command)
 
 
-def _mutateScript(context, script, suffix=""):
+def _mutateCommandline(context, commandline):
+    return _wrap_command(context, commandline)
+
+
+def _mutateScript(context, script):
     def mutate(context, command):
-        return _mutateCommandline(context, command, suffix)
+        return _mutateCommandline(context, command)
     return testplan.mutateScript(context, script, mutate)
 
 
+def remote_read_result_file(context, path):
+    assert os.path.isabs(path)
+    command = _wrap_command(context, "cat '%s'" % path)
+    logging.info("$ %s", command)
+    return subprocess.check_output(command, shell=True)
+
+
 def mutatePlan(context, plan):
-    plan.preparescript = _mutateScript(context, plan.preparescript, "-prepare")
+    plan.preparescript = _mutateScript(context, plan.preparescript)
+    # We need the temporary directory to exist on the remote as well.
+    command = _wrap_command(context,
+                            "mkdir -p '%s'" % os.path.dirname(context.tmpBase))
+    plan.preparescript.insert(0, command)
     plan.runscript = _mutateScript(context, plan.runscript)
+    plan.verifyscript = _mutateScript(context, plan.verifyscript)
+    for name, script in plan.metricscripts.items():
+        plan.metricscripts[name] = _mutateScript(context, script)
+
+    # Merging profile data should happen on the host because that is where
+    # the toolchain resides, however we have to retrieve the profile data
+    # from the device first, add commands for that to the profile script.
+    for path in plan.profile_files:
+        assert os.path.isabs(path)
+        command = "scp %s:%s %s" % (context.config.remote_host, path, path)
+        plan.profilescript.insert(0, command)
+
+    assert context.read_result_file is testplan.default_read_result_file
+    context.read_result_file = remote_read_result_file

litsupport/modules/timeit.py

 from litsupport import shellcommand
 from litsupport import testplan
 import os
 import re
 
 
 def _mutateCommandLine(context, commandline):
-    outfile = context.tmpBase + ".out"
     timefile = context.tmpBase + ".time"
     config = context.config
     cmd = shellcommand.parse(commandline)
 
     timeit = "%s/tools/timeit-target" % config.test_source_root
     args = ["--limit-core", "0"]
     args += ["--limit-cpu", "7200"]
     args += ["--timeout", "7200"]
if not config.traditional_output:
             if not os.path.isabs(stderr) and workdir is not None:
                 stderr = os.path.join(workdir, stderr)
             args += ["--redirect-stderr", stderr]
             cmd.stderr = None
     else:
         if cmd.stdout is not None or cmd.stderr is not None:
             raise Exception("Separate stdout/stderr redirection not " +
                             "possible with traditional output")
+        outfile = context.tmpBase + ".out"
         args += ["--append-exitstatus"]
         args += ["--redirect-output", outfile]
     stdin = cmd.stdin
     if stdin is not None:
         if not os.path.isabs(stdin) and workdir is not None:
             stdin = os.path.join(workdir, stdin)
         args += ["--redirect-input", stdin]
         cmd.stdin = None
def _mutateScript(context, script):
     if not hasattr(context, "timefiles"):
         context.timefiles = []
     return testplan.mutateScript(context, script, _mutateCommandLine)
 
 
 def _collectTime(context, timefiles, metric_name='exec_time'):
     time = 0.0
     for timefile in timefiles:
-        time += getUserTime(timefile)
+        filecontent = context.read_result_file(context, timefile)
+        time += getUserTimeFromContents(filecontent)
     return {metric_name: time}
 
 
 def mutatePlan(context, plan):
     if len(plan.runscript) == 0:
         return
     context.timefiles = []
     plan.runscript = _mutateScript(context, plan.runscript)
     plan.metric_collectors.append(
         lambda context: _collectTime(context, context.timefiles)
     )
 
 
 def getUserTime(filename):
-    """Extract the user time form a .time file produced by timeit"""
+    """Extract the user time from a .time file produced by timeit"""
     with open(filename) as fd:
-        line = [line for line in fd.readlines() if line.startswith('user')]
+        contents = fd.read()
+        return getUserTimeFromContents(contents)
+
+
+def getUserTimeFromContents(contents):
+    line = [line for line in contents.splitlines() if line.startswith('user')]
     assert len(line) == 1
 
     m = re.match(r'user\s+([0-9.]+)', line[0])
     return float(m.group(1))

litsupport/test.py

 import litsupport.testplan
 import os
 
 
 SKIPPED = lit.Test.ResultCode('SKIPPED', False)
 NOEXE = lit.Test.ResultCode('NOEXE', True)
 
 
-class TestContext:
-    """This class is used to hold data used while constructing a testrun.
-    For example this can be used by modules modifying the commandline with
-    extra instrumentation/measurement wrappers to pass the filenames of the
-    results to a final data collection step."""
-    def __init__(self, test, litConfig, tmpDir, tmpBase):
-        self.test = test
-        self.config = test.config
-        self.litConfig = litConfig
-        self.tmpDir = tmpDir
-        self.tmpBase = tmpBase
-
-
 class TestSuiteTest(lit.formats.ShTest):
     def __init__(self):
         super(TestSuiteTest, self).__init__()
 
     def execute(self, test, litConfig):
         config = test.config
         if config.unsupported:
             return lit.Test.Result(lit.Test.UNSUPPORTED, 'Test is unsupported')
         if litConfig.noExecute:
             return lit.Test.Result(lit.Test.PASS)
 
         # Parse .test file and initialize context
         tmpDir, tmpBase = lit.TestRunner.getTempPaths(test)
         lit.util.mkdir_p(os.path.dirname(tmpBase))
-        context = TestContext(test, litConfig, tmpDir, tmpBase)
+        context = litsupport.testplan.TestContext(test, litConfig, tmpDir,
+                                                  tmpBase)
         litsupport.testfile.parse(context, test.getSourcePath())
         plan = litsupport.testplan.TestPlan()
 
         # Report missing test executables.
         if not os.path.exists(context.executable):
             return lit.Test.Result(NOEXE, "Executable '%s' is missing" %
                                    context.executable)
 

litsupport/testplan.py

class TestPlan(object):
     metric_collectors contains a list of functions executed after the scripts
     finished."""
     def __init__(self):
         self.runscript = []
         self.verifyscript = []
         self.metricscripts = {}
         self.metric_collectors = []
         self.preparescript = []
+        self.profile_files = []
         self.profilescript = []
 
 
 def mutateScript(context, script, mutator):
     """Apply `mutator` function to every command in the `script` array of
     strings. The mutator function is called with `context` and the string to
     be mutated and must return the modified string. Sets `context.tmpBase`
     to a path unique to every command."""
def check_output(commandline, *aargs, **dargs):
     logging.info(" ".join(commandline))
     return subprocess.check_output(commandline, *aargs, **dargs)
 
 
 def check_call(commandline, *aargs, **dargs):
     """Wrapper around subprocess.check_call that logs the command."""
     logging.info(" ".join(commandline))
     return subprocess.check_call(commandline, *aargs, **dargs)
+
+
+def default_read_result_file(context, path):
+    with open(path) as fd:
+        return fd.read()
+
+
+class TestContext:
+    """This class is used to hold data used while constructing a testrun.
+    For example this can be used by modules modifying the commandline with
+    extra instrumentation/measurement wrappers to pass the filenames of the
+    results to a final data collection step."""
+    def __init__(self, test, litConfig, tmpDir, tmpBase):
+        self.test = test
+        self.config = test.config
+        self.litConfig = litConfig
+        self.tmpDir = tmpDir
+        self.tmpBase = tmpBase
+        self.read_result_file = default_read_result_file

tools/CMakeLists.txt

-# Note that we have to compile fpcmp and timeit for the host machine even when
-# cross compiling to a different target. We use custom rules doing a simple
-# "cc file.c".
+# 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)
 
-llvm_add_host_executable(build-fpcmp fpcmp fpcmp.c)
+add_executable(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)
if(TEST_SUITE_USE_PERF)
   add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/timeit-target
     COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_SOURCE_DIR}/timeit.sh ${CMAKE_CURRENT_BINARY_DIR}/timeit-target
     COMMAND chmod +x ${CMAKE_CURRENT_BINARY_DIR}/timeit-target
     DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/timeit.sh)
   add_custom_target(build-timeit-target DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/timeit-target)
 else()
   add_executable(timeit-target ${CMAKE_CURRENT_SOURCE_DIR}/timeit.c)
   add_executable(build-timeit-target ALIAS timeit-target)
   llvm_add_host_executable(build-timeit timeit timeit.c)
 endif()

utils/rsync.sh

+#!/bin/bash
+# Sync a build directory to remote device for running.
+set -eu
+DEVICE="$1"
+BUILDDIR="$2"
+
+case $BUILDDIR in
+    /*) ;;
+    *)
+        echo 1>&2 "Builddir path must be absolute!"
+        exit 1
+        ;;
+esac
+
+RSYNC_FLAGS=""
+RSYNC_FLAGS+=" -a"
+RSYNC_FLAGS+=" --delete --delete-excluded"
+# We cannot easily differentiate between intermediate build results and
+# files necessary to run the benchmark, so for now we just exclude based on
+# some file extensions...
+RSYNC_FLAGS+=" --exclude=\"*.o\""
+RSYNC_FLAGS+=" --exclude=\"*.a\""
+RSYNC_FLAGS+=" --exclude=\"*.time\""
+RSYNC_FLAGS+=" --exclude=\"*.cmake\""
+RSYNC_FLAGS+=" --exclude=Output/"
+RSYNC_FLAGS+=" --exclude=.ninja_deps"
+RSYNC_FLAGS+=" --exclude=.ninja_log"
+RSYNC_FLAGS+=" --exclude=build.ninja"
+RSYNC_FLAGS+=" --exclude=rules.ninja"
+RSYNC_FLAGS+=" --exclude=CMakeFiles/"
+
+set -x
+ssh $DEVICE mkdir -p "$BUILDDIR"
+eval rsync $RSYNC_FLAGS $BUILDDIR/ $DEVICE:$BUILDDIR/