Page MenuHomePhabricator

[test-suite] SPEC2017 CPU CAM4 floating point tests.
ClosedPublic

Authored by naromero77 on Feb 25 2021, 9:34 PM.

Details

Summary

Add SPEC2017 CPU CAM4 floating point rate and speed test.

Event Timeline

naromero77 created this revision.Feb 25 2021, 9:34 PM
naromero77 requested review of this revision.Feb 25 2021, 9:34 PM
Meinersbur added inline comments.Feb 25 2021, 11:33 PM
External/SPEC/CFP2017rate/527.cam4_r/CMakeLists.txt
28

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.

48–52

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.

61

[very nit] double space

62

target_compile_definitions would be preferred.

71

[typo] recompilde

577–578

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.

naromero77 added inline comments.Mar 1 2021, 2:45 PM
External/SPEC/CFP2017rate/527.cam4_r/CMakeLists.txt
28

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.

48–52

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.

naromero77 updated this revision to Diff 327346.Mar 1 2021, 8:26 PM
  • Use plain vanilla diff instead of specdiff.
  • Use target_compile_definitions instead.
  • Fix typo and clarify comment.
  • Remove redundant explicit dependencies.
Meinersbur accepted this revision.Mar 1 2021, 9:14 PM
Meinersbur added inline comments.
External/SPEC/CFP2017rate/527.cam4_r/CMakeLists.txt
28

Windows isn't supported by the test-suite anyway.

48–52

Could you point me the online sites you found?

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

59

[very nit] double space

This revision is now accepted and ready to land.Mar 1 2021, 9:14 PM
naromero77 added inline comments.Mar 1 2021, 9:29 PM
External/SPEC/CFP2017rate/527.cam4_r/CMakeLists.txt
48–52

Here is the most recent one which is still open:
https://gitlab.kitware.com/cmake/cmake/-/issues/20181

Here is another one, where the original issue was allegedly fixed:
https://gitlab.kitware.com/cmake/cmake/-/issues/17601

Here is an older one though that suggests its a build specific problem:
https://github.com/ninja-build/ninja/issues/543

59

Oops, sorry I missed the last double space. Will fix.

naromero77 updated this revision to Diff 327355.Mar 1 2021, 9:33 PM
  • Remove extraneous space.
This revision was automatically updated to reflect the committed changes.