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

Wed Mar 31 12:08:32 PDT 2021

Meinersbur added inline comments.

================
Comment at: External/SPEC/CFP2017speed/628.pop2_s/CMakeLists.txt:12
+macro(pop2_prepare_rundir)
+  # Similar to the spec2017_prepare_rundir with an input file fix.
+  # input file for pop2 is hard-coded to drv_in, but actual filename
----------------
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 ()
> ```

================
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.
----------------
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 ()
```

================
Comment at: External/SPEC/CFP2017speed/628.pop2_s/CMakeLists.txt:35
+
+# Compiler definitions affect C and *.f90 files, but note *.F90 files.
+# Following flags get added automatically by SpecCPU2017.cmake and not
----------------
[typo] not~~e~~

================
Comment at: External/SPEC/CFP2017speed/628.pop2_s/CMakeLists.txt:37
+# Following flags get added automatically by SpecCPU2017.cmake and not
+# include below:
+# -DSPEC -DSPEC_CPU -DNDEBUG
----------------
The value of `SPEC_AUTO_BYTEORDER` depends on the host machine. Some other are added as well. Not sure it makes sense to list the generic SPEC definitions here.

================
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(
----------------
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?

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

================
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)
+
----------------
If it works without `IGNORE_WHITESPACE`, is it even needed?

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

================
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)
----------------
Could this somehow be shared with 527.cam4_r, e.g. by moving both into `SpecCPU2017.cmake`?

================
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 ()
----------------
`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?

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

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

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

Repository:
  rT test-suite

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

https://reviews.llvm.org/D99640