[PATCH] D36717: [test-suite] Add SPEC CPU 2017

Matthias Braun via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Aug 15 14:02:38 PDT 2017


MatzeB added a comment.

In https://reviews.llvm.org/D36717#842495, @Meinersbur wrote:

> Thank you for the review!
>
> In https://reviews.llvm.org/D36717#841501, @MatzeB wrote:
>
> > - I'd suggest to remove some of the comments in the CMakeLists.txt files:
> >   - The link to the benchmark descriptions add little value to the cmake code itself (some of them are also wrong).
>
>
> Some links include hints about how the benchmarks are to be compiled. For instance, perlbench requires `-fno-strict-aliasing` which explains why the option as added. gcc requires either `-fgnu89-inline` or `-z muldefs` added to the linker. Hence the links contain information on why the options are set as they are.
>
> On the conterside, I don't see why the comments are problematic, even if they add only little value. That's at least some value.


They are not problematic. I mainly wanted to confirm that you indeed find them useful. Leave them in.

> Please respond whether I should still remove them.
> 
> Of course I will correct the wrong links.
> 
>> - The `COMP:` and `LINK:` and test execution lines were probably a big help for you bringing up the cmake files. But keeping them around now seems strange: for example I don't care that your compiler was installed in `/root/build/llvm/release/bin/` and there are deviations in the flags that are actually used from the flags present in the COMP/LINK lines. So I'd expect that to add more confusion than being helpful.
> 
> I agree, and will remove them in the next update. I wanted to remove them anyway, but at the moment when I run the ref datasets, I hit lit's default timout for xz_s and imgemagick_s on my computer. I only keep them until I know that all the verifications pass.





================
Comment at: External/SPEC/CMakeLists.txt:1-22
+include(External)
+
 add_subdirectory(CINT95)
 add_subdirectory(CINT2000)
 add_subdirectory(CFP2000)
 add_subdirectory(CINT2006)
 add_subdirectory(CFP2006)
----------------
Meinersbur wrote:
> MatzeB wrote:
> > It would be good if you could keep the `include(External)`/`externals_find` logic inside the subdirectories. The motivation for that is that people like running parts of the test-suite independently. As an example we currently have 4 bots running on our side that set TEST_SUITE_SUBDIRS=(External/SPEC/CINT200|External/SPEC/CINT2006 etc.
> > The problem with that approach is that cmake will not include the intermediate CMakeLists.txt. So you won't see any effects of `External/CMakeLists.txt` or `External/SPEC/CMakeLists.txt` but you will just see `External/SPEC/CINT2000/CMakeLists.txt` etc. so we have to design this in a way that the intermediate cmake files only use add_subdirectory() commands and do not set variables or otherwise modify state.
> > 
> > Putting common code into something like SpecCPU2017.cmake and using that from all subdirectories is the way to get there, you just have to do it for everything including the llvm_external_find call.
> That's a good idea.
> 
> Btw, SPEC2006/SPEC2000 uses `llvm_externals_find` in their CINT200x/CFP200x directories such that one cannot only configure an individual benchmark. It also prints the "Expected SPEC200x version 1.y version multiple times", which annoys me a bit (I don't have access to the latest versions).
> 
> Could we make "lnt runtest test-suite --only-test" one day use `TEST_SUITE_SUBDIRS` such that it configures only that particular directory instead of the entire test-suite?
> 
> 
Yes the current situation is far from perfect, it only works one level up, not for individual benchmarks. I mainly wanted to point out why things are organized the way they are for SPEC.

> It also prints the "Expected SPEC200x version 1.y version multiple times", which annoys me a bit (I don't have access to the latest versions).
I never realized that as I had the latest version, patches welcome. You could probably track in a variable like `SPEC2000_VERSION_WARNING_DISPLAYED` whether we already printed such a message.

> Could we make "lnt runtest test-suite --only-test" one day use TEST_SUITE_SUBDIRS such that it configures only that particular directory instead of the entire test-suite?
Several notes here:
- You can use `lnt --cmake-define TEST_SUITE_SUBDIRS=xxx` today, we do that in some of our (internal) bots and it works fine.
- SingleSource benchmarks cannot be selected just with directory names; I don't see how we can enforece or check that intermediate CMakeLists.txt do not change global state.
- Also see https://reviews.llvm.org/D34132 for my take/opinion on `lnt runtest test-suite`.


================
Comment at: External/SPEC/SpecCPU2017.cmake:119-122
+  if (DEFINED _arg_ORIGIN_PATH)
+    set(_arg_ORIGIN_PATH)
+    include("${ORIGIN_PATH}/CMakeLists.txt")
+  else ()
----------------
Meinersbur wrote:
> MatzeB wrote:
> > I think the ORIGIN_PATH logic would be better expressed by have a separate macro to call instead of setting a variable that then changes all the behavior of this macro.
> Could you outline how you think this should be structured?
> 
> The "problem" is that the ORIGIN_PATH must be already known in `cpu2017_benchmark` which is uses to "inherit" its source and data path from.
> 
> I could specify the origin benchmarks in both calls, but that information is redundant.
This is mostly a design "smell", in my experience there are often better solution possible if functions/macros have overly generic/uninformative names. Note that this is just a hunch and not always true, so I'm mainly asking questions to have you at least think about it :)

That said, even if you cannot eliminate the ORIGIN_PATH from the early `cpu2017_benchmark` for now, you could still a macro like `cpu2017_include_origin()` that should be used instead of `cpu2017_benchmark_done()` in the ORIGIN_PATH cases.


================
Comment at: External/SPEC/SpecCPU2017.cmake:150-156
+    if (EXISTS "${SRC_DIR}/image_validator")
+      file(GLOB_RECURSE VALIDATOR_SOURCES ${SRC_DIR}/image_validator/*.c ${SRC_DIR}/image_validator/*.cpp ${SRC_DIR}/image_validator/*.cc ${SRC_DIR}/image_validator/*.C)
+      add_executable(${VALIDATOR} ${VALIDATOR_SOURCES})
+      target_link_libraries(${VALIDATOR} m)
+      add_dependencies(${PROG} ${VALIDATOR})
+      test_suite_add_build_dependencies(${VALIDATOR})
+    endif ()
----------------
Meinersbur wrote:
> MatzeB wrote:
> > Does this mean some benchmark come with a custom tool to validate the results? I guess the current approach will fail in cross compilation settings. I don't have an easy solution for that though and we may want to address this in followup patches instead.
> Yes, each benchmark that outputs an image has its own image_validator tool. I did not compare their sources, maybe they are identical. `runspec` however even compiles the 5xx_r and 6xx_s variants sperately.
> 
> 
As for followup patches here:

Note that there is `llvm_add_host_executable` in `tools/CMakeLists.txt` which I consider a hack that happens to work well enough for timeit.c/fpcmp.c but will fall short if people start doing more complicated stuff in their host tools that require c flags, linker flags, etc.

Maybe we should move that into cmake/modules and use it here too, though moving it there also feels like legitimizing this hack.


================
Comment at: cmake/modules/TestFile.cmake:37-40
+  
+  if(DEFINED ARGS_WORKDIR)
+    set(ARGS_WORKDIR "cd ${ARGS_WORKDIR} ; ")
+  endif()
----------------
Meinersbur wrote:
> MatzeB wrote:
> > The WORKDIR feature seems unnecessary to me. Can't you rather prepend the workdir to the arguments of the fpcmp/diff command instead of adding this? Even if there is a reason to change the working directory, I think users should just prepend their command with the `cd xxx ; ` sequence. (I know `llvm_test_run` has the feature too, but that has more to do with the fact that people do not specify the executable for llvm_test_run, so there is no easy way to prepend something to the executable).
> The reason to change the workdir is that some of spec's verifiers (image_validator) put the input argument into the output. If it is invoked with an absolute path, the comparison with the reference output which does not include the absolute path will fail.
> 
> I found the WORKDIR parameter useful and cmake's `add_custom_target` itself features a `WORKING_DIRECTORY` argument, so I would argue it makes sense for consistence; and callers do not have to think about e.g. whether calling `cd` is platform-neutral.
> 
> However, it is not a big thing so I will remove it in the update.
I don't feel strongly about, but note that looking up whether a WORKING_DIRECTORY feature exists is probably equally complicated as determining whether `cd xxx` will work. You probably end up just scouting for examples/precedence in both cases. And when we have the `cd` feature working anyway, this feels like just introducing a 2nd syntax.


https://reviews.llvm.org/D36717





More information about the llvm-commits mailing list