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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


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


================
Comment at: External/SPEC/SpecCPU2017.cmake:123-148
+    set(_abserror)
+    if (DEFINED ABSOLUTE_TOLERANCE)
+      set(_abserror -a "${ABSOLUTE_TOLERANCE}")
+    endif ()
+    
+    set(_relerror)
+    if (DEFINED RELATIVE_TOLERANCE)
----------------
This is a bit subjective but I'd lean towards splitting this into two macros for test file creation and defining the test executable. While this is an extra line for each benchmark, I think it is easier to understand what is going on and allows you to choose meaningful macro names over an uninformative `benchmark_done` name...


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


================
Comment at: External/SPEC/SpecCPU2017.cmake:158-163
+    if (COPY_TO_RUNDIR)
+      foreach (_runtype IN LISTS TEST_SUITE_RUN_TYPE)  
+        llvm_copy_dir(${PROG} ${RUN_${_runtype}_DIR} "${INPUT_all_DIR}")
+        llvm_copy_dir(${PROG} ${RUN_${_runtype}_DIR} "${INPUT_${_runtype}_DIR}")
+      endforeach ()
+    endif ()
----------------
`COPY_TO_RUNDIR` is another case where a separate macro would work better than setting a variable to influence the behavior here.


https://reviews.llvm.org/D36717