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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

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



================
Comment at: External/SPEC/CINT2017rate/505.mcf_r/CMakeLists.txt:1
+# https://www.spec.org/cpu2017/Docs/benchmarks/520.omnetpp_r.html
+include(../../SpecCPU2017.cmake)
----------------
wrong link


================
Comment at: External/SPEC/CINT2017speed/605.mcf_s/CMakeLists.txt:1
+# https://www.spec.org/cpu2017/Docs/benchmarks/620.omnetpp_s.html
+include(../../SpecCPU2017.cmake)
----------------
wrong link?


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


================
Comment at: External/SPEC/SpecCPU2017.cmake:170
+# Compare an image file to a reference image.
+macro(validate_image _imgfile _cmpfile _outfile)
+  cmake_parse_arguments(_carg "" "RUN_TYPE" "SUITE_TYPE" ${ARGN})
----------------
I think it would be a good idea add a `spec_` (or `_spec`) to the name, so people know immediately that this is not a generic macro but contains logic designed for the spec benchmarks. The same applies to `add_include_dirs`, `set_sources`, `run_test`.


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


================
Comment at: litsupport/shellcommand.py:149
             continue
+        cmd.executable = os.path.join(cwd, cmd.executable)
         # We only support one executable yet for collecting md5sums
----------------
Modifying `cmd.executable` feeld wrong, the value should rather be pulled out into a variable and that variable modified.


================
Comment at: tools/fpcmp.c:249-264
+    while (F1P < File1End && F2P < File2End) {
+      if (*F1P == *F2P) {
+        ++F1P, ++F2P;
+        continue;
+			}
+
+      // Ignore CR, to be able to compare LF and CRLF formatted
----------------
I think this should go into a separate review. I think we also shouldn't make `\r` skipping the default behavior and rather require the user to enable an option.


https://reviews.llvm.org/D36717