[PATCH] D25575: test-suite: try to force many FP-dependent tests to compile with unaggressive FP optimization or not at all, add support for per-ISA reference outputs

[Matthias Braun via llvm-commits]

MatzeB added a comment.

I would accept the FAST_MATH changes on their own, however I don't want to accept the fp-contract stuff while the discussion about that is still ongoing.



================
Comment at: CMakeLists.txt:185
 
+execute_process(COMMAND echo ${CMAKE_C_COMPILER} ${CMAKE_CXX_COMPILER} ${CMAKE_C_FLAGS} ${CMAKE_CXX_FLAGS} ${CMAKE_C_COMPILER_ARG1} ${CMAKE_C_COMPILER_ARG2} ${CMAKE_C_COMPILER_ARG3} ${CMAKE_C_COMPILER_ARG4} ${CMAKE_C_COMPILER_ARG5} ${CMAKE_CXX_COMPILER_ARG1} ${CMAKE_CXX_COMPILER_ARG2} ${CMAKE_CXX_COMPILER_ARG3} ${CMAKE_CXX_COMPILER_ARG4} ${CMAKE_CXX_COMPILER_ARG5} COMMAND grep -E "(-Ofast|-ffast-math)" OUTPUT_VARIABLE FAST_MATH_DETECTED)
+
----------------
I would rename the final variable to something like "TEST_SUITE_USES_FAST_MATH" (so the name is also correct in the case of the variable being set by the user instead of automatically detected). From that point you should be able to handle it in a similar fashion as TEST_SUITE_BENCHMARKING_ONLY.


================
Comment at: MultiSource/Applications/minisat/CMakeLists.txt:1
-set(PROG minisat)
-list(APPEND LDFLAGS -lstdc++ -lm)
-if(DEFINED SMALL_PROBLEM_SIZE)
-  set(RUN_OPTIONS -verbosity=0 ${CMAKE_CURRENT_SOURCE_DIR}/small.cnf)
-else()
-  if(DEFINED LARGE_PROBLEM_SIZE)
-    set(RUN_OPTIONS -verbosity=0 ${CMAKE_CURRENT_SOURCE_DIR}/long.cnf)
-    if(ARCH STREQUAL "XCore")
-      set(XCORE_TARGET_NEEDS_MEMORY 64)
-    endif()
+if(FAST_MATH_DETECTED STREQUAL "")
+
----------------
This can simply be `if(NOT TEST_SUITE_USES_FAST_MATH)` (or however you call the variable).

You could also consider applying at the points where `llvm_add_subdirectories()` is called (grep for how `TEST_SUITE_BENCHMARKING_ONLY` is used, this should simply some things like the TSVC benchmark where you could simply exclude the TSVC directory at the toplevel).


================
Comment at: cmake/modules/SingleMultiSource.cmake:103-126
     if(NOT DEFINED NO_REFERENCE_OUTPUT)
-      if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${name}.reference_output.${ENDIAN}-endian.${KEY})
-        set(REFERENCE_OUTPUT ${CMAKE_CURRENT_SOURCE_DIR}/${name}.reference_output.${ENDIAN}-endian.${KEY})
-      elseif(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${name}.reference_output.${KEY})
-        set(REFERENCE_OUTPUT ${CMAKE_CURRENT_SOURCE_DIR}/${name}.reference_output.${KEY})
-      elseif(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${name}.reference_output.${ENDIAN}-endian)
-        set(REFERENCE_OUTPUT ${CMAKE_CURRENT_SOURCE_DIR}/${name}.reference_output.${ENDIAN}-endian)
-      elseif(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${name}.reference_output)
-        set(REFERENCE_OUTPUT ${CMAKE_CURRENT_SOURCE_DIR}/${name}.reference_output)
+      set(REF_OUTPUT_CANDIDATE_FILENAME_1 ${CMAKE_CURRENT_SOURCE_DIR}/${name}.reference_output.${ARCH}.${KEY})
+      set(REF_OUTPUT_CANDIDATE_FILENAME_2 ${CMAKE_CURRENT_SOURCE_DIR}/${name}.reference_output.${ARCH})
+      set(REF_OUTPUT_CANDIDATE_FILENAME_3 ${CMAKE_CURRENT_SOURCE_DIR}/${name}.reference_output.${ENDIAN}-endian.${KEY})
+      set(REF_OUTPUT_CANDIDATE_FILENAME_4 ${CMAKE_CURRENT_SOURCE_DIR}/${name}.reference_output.${KEY})
+      set(REF_OUTPUT_CANDIDATE_FILENAME_5 ${CMAKE_CURRENT_SOURCE_DIR}/${name}.reference_output.${ENDIAN}-endian)
+      set(REF_OUTPUT_CANDIDATE_FILENAME_6 ${CMAKE_CURRENT_SOURCE_DIR}/${name}.reference_output)
----------------
While this looks like a useful addition, it does not appear to be used here, is it?


https://reviews.llvm.org/D25575