[PATCH] D97527: [test-suite] SPEC2017 CPU CAM4 floating point tests.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: External/SPEC/CFP2017rate/527.cam4_r/CMakeLists.txt:27
+  # perl script that does not require installation of SPEC
+  set(SPECDIFF_BIN "${TEST_SUITE_SPEC2017_ROOT}/bin/specdiff")
+
----------------
`bin/specdiff` is a shell script that executes `specperl`. It contains
```
if [ ! -x "${_bin}/specperl" ]; then
    echo "specperl is not in ${_bin}; has the benchmark tree been installed?"
    exit 1
fi
```

Are you sure it doesn't need to be installed?

It it is too much effort to support a root dir that is not a SPEC installation, I would compromise on that. It would also mean that CPU2017 cannot be put into the repository's `test-suite-externals` directory anymore and expected to work on any platform.


================
Comment at: External/SPEC/CFP2017rate/527.cam4_r/CMakeLists.txt:47-51
+# two binaries (exe,validator) are in the same directory with
+# overlapping source and ninja gives a hard error during the depency
+# analysis because it leads to duplicate rules for common modules
+# modules. Create a OBJECT library from netcdf sources and re-use the
+# this target
----------------
The same source used for multiple artifacts is supposed to work for C/C++ (e.g. often used to build a shared and static library). Is this a bug in CMake's Fortran support?

Another thing you could try to build the validator in another CMakeLists.txt added by `add_subdirectory`. Different add_subdirectories have independent dependency analyses. An (object) library might still be preferable to not compile these files twice.


================
Comment at: External/SPEC/CFP2017rate/527.cam4_r/CMakeLists.txt:60
+
+  add_library(netcdf  OBJECT ${netcdf_sources})
+  set_target_properties(netcdf PROPERTIES COMPILE_FLAGS "-DSPEC")
----------------
[very nit] double space


================
Comment at: External/SPEC/CFP2017rate/527.cam4_r/CMakeLists.txt:61
+  add_library(netcdf  OBJECT ${netcdf_sources})
+  set_target_properties(netcdf PROPERTIES COMPILE_FLAGS "-DSPEC")
+endif ()
----------------
`target_compile_definitions` would be preferred.


================
Comment at: External/SPEC/CFP2017rate/527.cam4_r/CMakeLists.txt:70
+
+# speed tests is recompilde, but also needs to find the netcdf modules
+# that are in the rate build directory
----------------
[typo] recompil**d**e


================
Comment at: External/SPEC/CFP2017rate/527.cam4_r/CMakeLists.txt:576-577
+)
+add_dependencies(${VALIDATOR} netcdf)
+target_link_libraries(${PROG} $<TARGET_OBJECTS:netcdf>)
+
----------------
According to https://cmake.org/cmake/help/latest/command/add_library.html#object-libraries, `$<TARGET_OBJECTS:netcdf>` would be added lie a source file, to `add_executable` not as a library dependency.

The manual also mentions `target_link_libraries(${PROG} PRIVATE netcdf)` which should be able to replace both of these lines.


Repository:
  rT test-suite

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97527/new/

https://reviews.llvm.org/D97527