[PATCH] D50209: cmake: Explicitely specify benchmark data

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: cmake/modules/TestSuite.cmake:9-25
+function(llvm_test_data target)
+  cmake_parse_arguments(_LTDARGS "" "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})
----------------
Meinersbur wrote:
> 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.
> 
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, ...)


================
Comment at: cmake/modules/TestSuite.cmake:16
+  foreach(file ${_LTDARGS_UNPARSED_ARGUMENTS})
+    llvm_copy(${target} $<TARGET_FILE_DIR:${target}>/${file} ${SOURCE_DIR}/${file})
+  endforeach()
----------------
Meinersbur wrote:
> 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.
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).


================
Comment at: cmake/modules/TestSuite.cmake:20
+
+function(llvm_test_data_dir target)
+  set(SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
----------------
Meinersbur wrote:
> Could `llvm_test_data` automatically determine whether the argument is a directory/file?
Good idea, that should be possible.


Repository:
  rT test-suite

https://reviews.llvm.org/D50209