[PATCH] D99640: [test-suite] SPEC2017 CPU pop2 floating point test.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: External/SPEC/CFP2017speed/628.pop2_s/CMakeLists.txt:105
+# SPEC Test has both absolute and relative tolerance with same order
+speccpu2017_verify_output(IGNORE_WHITE_SPACE ABSOLUTE_TOLERANCE 0.03 RELATIVE_TOLERANCE 0.03)
+
----------------
naromero77 wrote:
> Meinersbur wrote:
> > If it works without `IGNORE_WHITESPACE`, is it even needed?
> hmmm... it might a difference with the older version of the floating point comparison utility.
> 
> We can take it out.
> 
Does D99619 always ignore whitespace? That would be a bug.


================
Comment at: External/SPEC/CFP2017speed/628.pop2_s/CMakeLists.txt:109
+# directory
+speccpu2017_add_executable(
+  netcdf/attr.c
----------------
naromero77 wrote:
> Meinersbur wrote:
> > Is listing the files needed (e.g. because some files must not linked into the executable) or could it also be done with a `file(GLOB_RECURSIVE ...)`?
> This was quite tricky for me because I wanted to use `file(GLOB_RECURSIVE ...)`, but ran into a lot of trouble on several fronts.
> 
> The first issue was:
> `file(GLOB_RECURSIVE ..)` needs to run after you generate the preprocessed files with `speccpu2017_run_specpp`, not before. So, in my first attempt, I seem to create a race condition due to some dependency not being properly created.
> 
> The second issue was:
> There is a bunch of files that have `*.f90` that are `include` in other Fortran files and that should not be compiled.
> 
> Lastly, and perhaps I do not understand the CMake docs here:
> https://cmake.org/cmake/help/latest/command/file.html
> 
> It actually advises not to use GLOB to list source files:
> > We do not recommend using GLOB to collect a list of source files from your source tree. If no CMakeLists.txt file changes when a source is added or removed then the generated build system cannot know when to ask CMake to regenerate. The CONFIGURE_DEPENDS flag may not work reliably on all generators, or if a new generator is added in the future that cannot support it, projects using it will be stuck. Even if CONFIGURE_DEPENDS works reliably, there is still a cost to perform the check on every rebuild. 
> 
> 
> 
> 
> 
> 
First issue:
Can glob the unprocessed file names and derive the processed paths using get_filename_component. With the suggested combining of speccpu2017_add_executable and speccpu2017_run_specpp. this would be a single step.

Second issue:
Indeed a problem and why I also fell back to listing all source files explictly in some of the C/C++ benchmarks.

This issue:
It is indeed policy for llvm-project to list all files explicitly for this reason, but llvm-test-suite does this everywhere already and the benchmark source files are not expected to change for SPEC.


================
Comment at: External/SPEC/SpecCPU2017.cmake:374
+      OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/${_filename}"
+      COMMAND "${_specpp_bin}" ${ARGN} "${_absfilename}" > "${CMAKE_CURRENT_BINARY_DIR}/${_filename}"
+      DEPENDS "${_absfilename}"
----------------
naromero77 wrote:
> Meinersbur wrote:
> > [Idea] Use of `target_sources` to add the file to the executable instead of needing to list it in `speccpu2017_add_executable`
> > 
> > Files that need to go trough specpp could also be specified as a "SPECPP_SOURCES" argument of `speccpu2017_add_executable` and share the preprocessor definitions.
> Can
> 
> ```
> llvm_test_executable(${PROG} ${_sources})
> ```
> be called multiple times? This is called inside of `speccpu2017_add_executable` and defines the target `${PROG}` and then adds the sources. I think the answer is //no,// that it cannot be called multiple times -- instead, you are suggesting to use `target_sources` to append the sources to the already defined target, `${PROG}` ? 
> 
> I will give it a shot.
> 
> 
`llvm_test_executable` cannot be called multiple times.

If merging `speccpu2017_add_executable`  and `speccpu2017_run_specpp` as suggested in the second paragraph, the list of source files can also be assembled first, then `llvm_test_executable` called using that list.


Repository:
  rT test-suite

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

https://reviews.llvm.org/D99640