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

Tue Aug 15 13:25:18 PDT 2017

Meinersbur added a comment.

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.

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)
----------------
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?

================
Comment at: External/SPEC/SpecCPU2017.cmake:119-122
+  if (DEFINED _arg_ORIGIN_PATH)
+    set(_arg_ORIGIN_PATH)
+    include("${ORIGIN_PATH}/CMakeLists.txt")
+  else ()
----------------
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.

================
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 ()
----------------
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.

================
Comment at: cmake/modules/TestFile.cmake:37-40
+  
+  if(DEFINED ARGS_WORKDIR)
+    set(ARGS_WORKDIR "cd ${ARGS_WORKDIR} ; ")
+  endif()
----------------
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.

https://reviews.llvm.org/D36717