[PATCH] D50209: cmake: Explicitely specify benchmark data

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur 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})
----------------
MatzeB wrote:
> Meinersbur wrote:
> > MatzeB wrote:
> > > 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, ...)
> > 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.
> 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...
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. 


Repository:
  rT test-suite

https://reviews.llvm.org/D50209