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

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added inline comments.


================
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:
> 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.
I guess that this benchmark-specific image_validator is very close in intended use as fpcmp.c.
Therefore, my gut feel is that it'd probably be best if the fpcmp and the image_validator binaries were built using the same mechanism, even if it was extending a current hack.
Wouldn't 1 hack be preferable over 2 different hacks; so that when the hack needs to be fixed, only one location needs to be fixed?



https://reviews.llvm.org/D36717