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

Mon Apr 5 15:18:28 PDT 2021

naromero77 added inline comments.

================
Comment at: External/SPEC/CFP2017speed/628.pop2_s/CMakeLists.txt:12-15
+  # Similar to the spec2017_prepare_rundir with an input file fix.
+  # input file for pop2 is hard-coded to drv_in, but actual filename
+  # in SPEC2017 benchmark input directory is drv_in.in. The SPEC perl
+  # run script renames the file as well.
----------------
Meinersbur wrote:
> Meinersbur wrote:
> > How does the SPEC .pm driver handle this?
> > 
> > Did you consider invoking `speccpu2017_prepare_rundir` and just fix the file name after that (Instead of copy-pasting its code)?
> > ```
> > spec2017_prepare_rundir()
> > foreach (_runtype IN LISTS TEST_SUITE_RUN_TYPE)
> >   llvm_copy(${PROG} "${RUN_${_runtype}_DIR}/drv_in" "${INPUT_${_runtype}_DIR}/drv_in.in")
> > endforeach ()
> > ```
> 
There is extra perl code to hand it. Sorry, for the duplicated code. I will fix it.

================
Comment at: External/SPEC/CFP2017speed/628.pop2_s/CMakeLists.txt:66
+# end up with a ridiculously long line that some compilers, like
+# GCC, are unable to compile.
+speccpu2017_run_specpp(
----------------
Meinersbur wrote:
> Do I understand correctly, gfortran has problems to compile long lines? Is this a problem with the vanilla SPEC as well if installed in a deeply nested directory?
Correct

It should also happen with plain vanilla SPEC, but you would have to intentionally create a deeply nested directory. 

================
Comment at: External/SPEC/CFP2017speed/628.pop2_s/CMakeLists.txt:83-84
+  -DSPEC_CASE_FLAG
+  -DSPEC_AUTO_BYTEORDER=${SPEC_AUTO_BYTEORDER_VALUE}
+  -DSPEC_${SPEC_PTR_TYPE}
+  -DSPEC_SUPPRESS_OPENMP
----------------
Meinersbur wrote:
> It is kind-of intransparent where SPEC_AUTO_BYTEORDER_VALUE and SPEC_PTR_TYPE come from. Did you consider assemble a list preprocessor definitions shared between all CPU2017 benchmarks and make `speccpu2017_run_specpp` add them implicitly (Liek they are added implicitly for the source files themselves)? Similaryl, `-U__FILE__` and `-w` don't seem to be specific to pop2.
I would have to do some analysis of the other tests like `cam4` and `wrf` to figure it out, but yes, it can be done. And would avoid some duplicated code. Thanks for the suggestion.

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

================
Comment at: External/SPEC/CFP2017speed/628.pop2_s/CMakeLists.txt:109
+# directory
+speccpu2017_add_executable(
+  netcdf/attr.c
----------------
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. 

================
Comment at: External/SPEC/CFP2017speed/628.pop2_s/CMakeLists.txt:437
+
+check_fortran_compiler_flag("-fallow-argument-mismatch" SUPPORTS_FALLOW_ARGUMENT_MISMATCH)
+if (SUPPORTS_FALLOW_ARGUMENT_MISMATCH)
----------------
Meinersbur wrote:
> Could this somehow be shared with 527.cam4_r, e.g. by moving both into `SpecCPU2017.cmake`?
Good idea.

================
Comment at: External/SPEC/CFP2017speed/628.pop2_s/CMakeLists.txt:458
+      "628.pop2_s not supported. No way to read big-endian binary IO on little-endian architecture.")
+    return ()
+  endif ()
----------------
Meinersbur wrote:
> `speccpu2017_verify_output` will have been called already at this point, only `pop2_prepare_rundir` is skipped when returning in this point, i.e. it will still try to run pop2 and fail because of files missing. Move this check to the beginning?
I can fix that.

================
Comment at: External/SPEC/SpecCPU2017.cmake:363
+
+# Run specpp on all Fortran files, but do not do it recursively
+macro(speccpu2017_run_specpp)
----------------
Meinersbur wrote:
> Could explain here why running specpp is needed (instead of the Fortran-compiler's integrated preprocessor)?
Yes, I can add that. 

================
Comment at: External/SPEC/SpecCPU2017.cmake:366-368
+  # We don't use GLOB_RECURSE below, because the stdout redirects
+  # don't include relative path of the source file. Hence, there could
+  # potentially be two files with the same name in different directories.
----------------
Meinersbur wrote:
> Could use `GLOB_RECURSIVE RELATIVE {SRC_DIR}` or `file(RELATIVE_PATH)` instead of get_filename_component to get a relative path. Might need `file(MAKE_DIRECTORY ` if specpp doesn't create directories itself.
> 
> This assumes that there even are files in subdirectories that need to be processed.
We could, but I think this will turn out to make no difference. 

I //think// that the files that need to be processed by `specpp` are in the top level directory and not any any subdirectories. The only subdirectory in the SPEC tests that require `specpp` are the `netcdf` subdirectory which doesn't actually need `specpp`.

Even so, maybe it would be good to just code up the general case if its not too hard. At the very least, I can update the comment. But I need to take a look at the other tests.

================
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}"
----------------
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.

Repository:
  rT test-suite

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

https://reviews.llvm.org/D99640