This is an archive of the discontinued LLVM Phabricator instance.

[test-suite] SPEC2017 CPU pop2 floating point test.
ClosedPublic

Authored by naromero77 on Mar 30 2021, 10:51 PM.

Details

Summary

Add the pop2 speed test:

  • Workaround for deeply nested subdirectories.
  • Add support for the specpp custom preprocessor.
  • Set portability flag variables in SpecCPU2017.cmake that are later passed into specpp.
  • Guard against running test on little-endian system with a compiler that does not support big-endian binary IO.
  • Requires https://reviews.llvm.org/D99619

Diff Detail

Repository
rT test-suite

Event Timeline

naromero77 created this revision.Mar 30 2021, 10:51 PM
naromero77 requested review of this revision.Mar 30 2021, 10:51 PM
Meinersbur added inline comments.Mar 31 2021, 12:08 PM
External/SPEC/CFP2017speed/628.pop2_s/CMakeLists.txt
13
13–16

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 ()
36

[typo] note

38

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.

67

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?

84–85

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.

106

If it works without IGNORE_WHITESPACE, is it even needed?

110

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 ...)?

438

Could this somehow be shared with 527.cam4_r, e.g. by moving both into SpecCPU2017.cmake?

459

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?

External/SPEC/SpecCPU2017.cmake
365

Could explain here why running specpp is needed (instead of the Fortran-compiler's integrated preprocessor)?

368–370

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.

376

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

naromero77 added inline comments.Apr 5 2021, 3:18 PM
External/SPEC/CFP2017speed/628.pop2_s/CMakeLists.txt
13–16

There is extra perl code to hand it. Sorry, for the duplicated code. I will fix it.

67

Correct

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

84–85

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.

106

hmmm... it might a difference with the older version of the floating point comparison utility.

We can take it out.

110

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.

438

Good idea.

459

I can fix that.

External/SPEC/SpecCPU2017.cmake
365

Yes, I can add that.

368–370

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.

376

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.

Meinersbur added inline comments.Apr 5 2021, 7:06 PM
External/SPEC/CFP2017speed/628.pop2_s/CMakeLists.txt
106

Does D99619 always ignore whitespace? That would be a bug.

110

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.

External/SPEC/SpecCPU2017.cmake
376

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.

naromero77 updated this revision to Diff 335998.Apr 7 2021, 9:50 PM
  • Use GLOB in speccpu2017_run_specpp
  • Move common definitions and flags used by specpp into SpecCPU2017.cmake
  • Re-use speccpu2017_prepare_rundir
  • Number of minor fixes
  • Number of updates to comments
naromero77 updated this revision to Diff 335999.Apr 7 2021, 9:57 PM

Trying to send commits again.

Was still missing earlier commits.

naromero77 updated this revision to Diff 336594.Apr 9 2021, 9:07 PM
  • big-endian flag fix for GCC and Intel.
Meinersbur added a comment.EditedApr 19 2021, 9:43 AM

I think I would still prefer a common list of compile definitions for specpp and add_definitions. I.e.

set(SPEC_COMMON_DEFS)
if (ENDIAN STREQUAL "little")
  list(APPEND SPEC_COMMON_DEFS "-DSPEC_AUTO_BYTEORDER=0x12345678")
elseif (ENDIAN STREQUAL "big")
  list(APPEND SPEC_COMMON_DEFS "-DSPEC_AUTO_BYTEORDER=0x87654321")
endif ()

[...]

add_definitions(${SPEC_COMMON_DEFS})

[...]

add_custom_command(
  COMMAND "${_specpp_bin}" ${SPEC_COMMON_DEFS} ...
  )
External/SPEC/CFP2017speed/628.pop2_s/CMakeLists.txt
162

What does the SHELL: do?

External/SPEC/SpecCPU2017.cmake
119–126
129–144
373

[change request] This is specific to pop2, i.e. should belong into 628.pop2_s/CMakeLists.txt

Suggestion: Pass a SOURCES option to speccpu2017_run_specpp, do the globbing in 628.pop2_s/CMakeLists.txt

  • Use a single shared variable for definitions that are used by specpp and regular cmake add_definitions function.
  • speccpu2017_run_specpp now takes two mandatory lists. one for files and the other for compiler definitions.
  • Add missing '>' to generator expression.
  • Glob pop2 source Fortran90 files that are passed into specpp in the pop2 test specific CMakeLists.txt.
Meinersbur accepted this revision.Apr 21 2021, 11:21 PM

LGTM, thank you.

This revision is now accepted and ready to land.Apr 21 2021, 11:21 PM
This revision was automatically updated to reflect the committed changes.