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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur accepted this revision.
Meinersbur added inline comments.
This revision is now accepted and ready to land.


================
Comment at: External/SPEC/CFP2017rate/527.cam4_r/CMakeLists.txt:59
+
+  add_library(netcdf  OBJECT ${netcdf_sources})
+  target_compile_definitions(netcdf PRIVATE SPEC)
----------------
[very nit] double space


================
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")
+
----------------
naromero77 wrote:
> Meinersbur wrote:
> > `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.
> You are right. It needs to be installed. I think that I have found a work around though. `specdiff` make sure that the CAM4 validator outputs says
> 
> ```
>  PASS:            3  points. 
> ```
> against the reference.
> 
> So, `specdiff <file>` is equivalent to `diff -b <file1> <file2>`, the `-b` just says ignore extra whitespace. `diff` is available with Linux and MacOS X. I am not sure about Windows.
Windows isn't supported by the test-suite anyway.


================
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
----------------
naromero77 wrote:
> Meinersbur wrote:
> > 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.
> Ninja came to a hard stop because of multiple rules generating the same module file error. I spent a while searching online for a work around. In some CMake issues, it looks like it was a known "behavior" with CMake + Ninja specific to Fortran. In previous version of ninja, it was only a warning, but now its a hard error. It looked like it was still possible to override this behavior with a optional flag to ninja. However since they are they same source files it was just as easy to make it an (object) library and to avoid the duplicate compile.
> 
> I did not try to build the validator in a separate CMakeLists.txt and perhaps indeed this would have solved my issue as well.
Could you point me the online sites you found?

However, I don't think it's worth looking for better workarounds.


Repository:
  rT test-suite

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

https://reviews.llvm.org/D97527