This is an archive of the discontinued LLVM Phabricator instance.

cmake: Explicitely specify benchmark data
ClosedPublic

Authored by MatzeB on Aug 2 2018, 5:27 PM.

Details

Summary

Explicitely declare what files are used as inputs for the benchmarks.
This changes the benchmarks to:

  • Copy the related input files next to the binary into the build folder.
  • Set the working directory of the benchmark to be the folder of the executable.
  • Having the data next to the binary also reduces the amount of absolute paths in the test files.

With this change in place you do not need the test-suite source
repository anymore to run the benchmarks. This is an important step
towards having a test-suite that can be cross-compiled on a host, then
copied onto a device and ran there without having a shared filesystem
like NFS setup.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB created this revision.Aug 2 2018, 5:27 PM
MatzeB added a comment.Aug 2 2018, 5:29 PM

Some notes:

  • I is only for the benchmarks shipping with the test-suite yet. The things in External will need some additional changes (they will keep working as is, just won't consistenly have their data in the builddir)
  • The copying grows the builddir from ~900MB to 1GB for me. If this is a concern then we could add a mode that creates symlinks when the test-suite is running locally; however I wasn't convinced yet it is worth having two modes just for 100MB savings in the builddir.
MatzeB updated this revision to Diff 158873.Aug 2 2018, 5:33 PM
  • properly document new cmake functions and add SUB_DIRECTORY argument to llvm_test_data_dir() for consistency.

Some notes:

  • I is only for the benchmarks shipping with the test-suite yet. The things in External will need some additional changes (they will keep working as is, just won't consistenly have their data in the builddir)
  • The copying grows the builddir from ~900MB to 1GB for me. If this is a concern then we could add a mode that creates symlinks when the test-suite is running locally; however I wasn't convinced yet it is worth having two modes just for 100MB savings in the builddir.

I am indeed concerned. The SPEC (2017) benchmark data can be >100MB per benchmark. In addition, I am adding the build dir to a git repository to see what the differences are (some file types ignored, eg object files). Having the input data in the build dir would blow up that repository, if these are symlinks.

I don't want to block this patch because of my particular setup, but it means that I need something more intelligent than some file extensions in the .gitignore of the build dir.

cmake/modules/TestSuite.cmake
9–25 ↗(On Diff #158871)

Could we work towards a 'one call=one benchmark specification' approach? Such as

llvm_test(progname
  SOURCES ...
  CFLAGS ...
  WORKDIR ...
  RUN_OPTIONS ...
  RUN_DATA ...
  ...

I always found the different functions/macros that have to be called in a specific order and pass state between each other (TESTSCRIPT) more complex than necessary.

16 ↗(On Diff #158871)

In case of SPEC 2017, a working directory is created where (when necessary) the run data is copied to and the program writes its data to. Some benchmarks construct the input file path from a sibling dir name (run_${runtype}) to argv[0] or the current cwd.

20 ↗(On Diff #158871)

Could llvm_test_data automatically determine whether the argument is a directory/file?

MatzeB updated this revision to Diff 158882.Aug 2 2018, 6:32 PM

Fix benchmarks I missed before because they aren't built by default.

MatzeB added inline comments.Aug 2 2018, 6:51 PM
cmake/modules/TestSuite.cmake
9–25 ↗(On Diff #158871)

Some points to this:

  • The current style of setting magic variables that are later read by llvm_singlesource()/llvm_multisource() is an effect of the original cmake build being a half-automatic translation from the makefiles. It's terrible style to have magic variables without obvious clues to the user that they have an effect on the two calls (rather than being temporary variables). We only do this style for llvm_singlesource()/llvm_multisource() and I consider it deprecated (= the ugly stuff is in cmake/modules/SingleMultiSource.cmake and considered deprecated.
  • The next interesting discussion is about:
llvm_test(name
  FOO ...
  BAR ...
  BAZ ...
)

compared to

llvm_test(name)
llvm_test_foo(name ...)
llvm_test_bar(name ...)
llvm_test_baz(name ....)

The former is a more compact syntax and avoids repeating the target name.
However it is bad for modularity/extensibility. The 2nd style makes it more natural to invent convenience functions (that are only used for some macros) and still have them naturally integrate into the flow of things without feeling out of place.

In practice I would aim for a compromise here: Only include the very basic options that the big majority of benchmarks will use in the basic command, and have everything else as an additional call on the target that you can perform afterwards... (say progname, SOURCES, CFLAGS in there and keep the test run/verification stuff in a separate command, ...)

16 ↗(On Diff #158871)

I guess we can experiment some more with symlinks (as in we symlink to the builddir and the copying is relative to this symlink...). That said I'd be fine to simply not convert SPEC2017 to the "everything in the build directory" style for now, it'll just keep working as is (the new style/not new style will only make a difference for setups when you test on external devices without having a shared filesystem).

20 ↗(On Diff #158871)

Good idea, that should be possible.

MatzeB updated this revision to Diff 158885.Aug 2 2018, 7:01 PM

Let llvm_test_data() handle directories so we don't need a separate llvm_test_data_dir().

Meinersbur accepted this revision.Aug 20 2018, 10:38 AM

I am ok with committing this, but maybe we should have someone else's opinion as well?

cmake/modules/TestSuite.cmake
9–25 ↗(On Diff #158871)

One of the problems with second style is that not all details are available in each llvm_test_*. For instance, e.g. SPEC 2017, have programs that have to be compiled for verification (such as convert compressed images to the plain format the reference images are in). To compile these programs, CFLAGS are needed as well.

However, this is not the topic of this patch.

16 ↗(On Diff #158871)

It would be nice if we could handle all benchmarks using the same mechanisms in some future.

This revision is now accepted and ready to land.Aug 20 2018, 10:38 AM
MatzeB updated this revision to Diff 161592.Aug 20 2018, 3:39 PM

Updated patch:

  • Update External benchmarks to specify llvm_test_data() as well.
  • Create a symbolic link for the data by default. The MUST_COPY argument to llvm_test_data() forces a copy where necessary. I plan to add a global setting in upcoming patches, that allow users to always force a copy instead of a symlink when running on an external device without shared filesystem.

I am ok with committing this, but maybe we should have someone else's opinion as well?

I'm not sure there are many other people around that care about how the test-suite cmake files are written. That said I think @cmatthews cares about being able to run the test-suite in a setup without NFS as well.

cmake/modules/TestSuite.cmake
9–25 ↗(On Diff #158871)

Not sure I completely understand the situation with spec2017, can you point me to a specific benchmark in spec2017 that shows this?

In general if there is a situations where steps need to communicate with each other such as passing along CFLAGS then we could attach them as properties to the target...

16 ↗(On Diff #158871)

My latest patch creates symlinks by default instead of copying the data. So there shouldn't be a reason anymore not to use llvm_test_data() for spec 2017 as well... (Though I will not do the transition right now as I cannot test things for spec 2017)

Meinersbur added inline comments.Aug 20 2018, 4:23 PM
cmake/modules/TestSuite.cmake
9–25 ↗(On Diff #158871)

511.povray_r, 526.blender_r, 538.imagick_r, 525.x264_r

These call speccpu2017_validate_image, which compiles a validator program. The validator program requires as least -DSPEC to be added, which is atm hardcoded into speccpu2017_validate_image.

One could indeed set all options as target properties and have a call at the end such as llvm_test_finished() (In SPEC2017 this would be speccpu2017_add_executable). However, the target must already have been created using add_executable. Some of the target's details cannot be changed after that, such as the target's name itself (even now it is possible to change alter the target name using the PREFIX argument, but it is not passed to the lit modules for collecting executable size, statistics, etc, which already has caused problems for me).

On the other hand, I agree that specifying multiple executions such as speccpu2017_run_test does is nicer to have different invocations for, instead of a single giant call that defines all RUN lines.

MatzeB added inline comments.Aug 20 2018, 4:30 PM
cmake/modules/TestSuite.cmake
9–25 ↗(On Diff #158871)

even now it is possible to change alter the target name using the PREFIX argument, but it is not passed to the lit modules for collecting executable size, statistics, etc, which already has caused problems for me

I recently changed this in r338622: PREFIX can only be used with llvm_singlesource() anymore, so here and in many other situations you can stop worrying :)

Meinersbur added inline comments.Aug 20 2018, 5:29 PM
cmake/modules/TestSuite.cmake
9–25 ↗(On Diff #158871)

The problem with PREFIX also applies to llvm_singlesource.

In my case, I added a newer version of Polybench (v4.2.1b) to the test-suite. Because its executable names clash with Polybench v3.2 which is already in a test-suite, I set the the PREFIX.

stats.py and compiletime.py used to identify the prefix by searching for the first dash in the executable name. In another commit (r286276) the dash was made optional.

The code to identify the correct object file is still:

prefix = ""
if context.config.single_source:
    prefix = "%s." % os.path.basename(context.executable)

The field context.executable is derived from the target name, i.e. includes the prefix. But the object file name is derived from the filename, i.e. does not contain the prefix. That is, no .o.time file will ever match if a prefix is used.

We are somewhat off-topic here. This is just to illustrate that some test arguments may be required for a different cmake-macro than it is passed to. Here, I ended up passing all benchmark source files in the .test file such that the python script can directly derive the .o.time or .stat file and I am sure that no such file is missed during a directory traversal. This required that one of the macros in TestFile.cmake is passed the list of source files, not just llvm_test_executable.

This revision was automatically updated to reflect the committed changes.