[PATCH] D50209: cmake: Explicitely specify benchmark data

Matthias Braun via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Aug 20 15:45:56 PDT 2018


MatzeB added a comment.

> 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.



================
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:
> 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...


================
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:
> MatzeB wrote:
> > 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).
> It would be nice if we could handle all benchmarks using the same mechanisms in some future.
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)


Repository:
  rT test-suite

https://reviews.llvm.org/D50209





More information about the llvm-commits mailing list