[builtins] Cleanup generic-file filtering

[rprichard]

Summary

Split filter_builtin_sources into two functions:

• filter_builtin_sources that removes generic files when an arch-specific file is selected.
• darwin_filter_builtin_sources that implements the EXCLUDE/INCLUDE lists (using the files in lib/builtins/Darwin-excludes).

darwin_filter_builtin_sources delegates to filter_builtin_sources.

Previously, lib/builtins/CMakeLists.txt had a number of calls to
filter_builtin_sources (with a confusing/broken use of the
excluded_list parameter), as well as a redundant arch-vs-generic
filtering for the non-Apple code path at the end of the file. Replace
all of this with a single call to filter_builtin_sources.

Remove i686_SOURCES. Previously, this list contained only the
arch-specific files common to 32-bit and 64-bit x86, which is a strange
set. Normally the ${ARCH}_SOURCES list contains everything needed for
the arch. "i686" isn't in ALL_BUILTIN_SUPPORTED_ARCH.

NFCI, but i686_SOURCES won't be defined, and the order of files in
${arch}_SOURCES lists will change.

[rprichard]

The filter_builtin_sources calls seems redundant with the non-Apple
code path at the end that detects arch-specific variants even if the
generic variants are filtered. The Apple code path also involves calls
to filter_builtin_sources, but I'm not sure if they're sufficient.

It looks like that code path was added last October in https://reviews.llvm.org/D69189. Maybe the filter_builtin_sources calls from this file can be removed. IIRC, I think the Apple pathway may be calling filter_builtin_sources anyway, so the calls here might be unnecessary.

IIRC, the non-ARM, non-x86 architectures aren't using filter_builtin_sources from this file.

[rprichard]

It looks like the current filter_builtin_sources calls were added in D37166.

[compnerd]

Why not just replace the filter_builtin_sources with list(REMOVE_ITEM)? It allows for a variadic number of parameters and avoids having to understand the nuance of filter_builtin_sources, as well as the NOTHING_TO_EXCLUDE variable.

[rprichard]

In D82151#2123058, @compnerd wrote:

Why not just replace the filter_builtin_sources with list(REMOVE_ITEM)? It allows for a variadic number of parameters and avoids having to understand the nuance of filter_builtin_sources, as well as the NOTHING_TO_EXCLUDE variable.

Would we add things like so:

list(REMOVE_ITEM x86_64_SOURCES
  floatdidf.c
  floatdisf.c
  floatdixf.c
)

It seems repetitive, I suppose? Each list of arch-specific files would need a mostly similar list of generic files to remove.

I read the Apple code path, and it looks like it always passes the ${arch}_SOURCES lists through filter_builtin_sources, so I think the calls to filter_builtin_sources from lib/builtins/CMakeLists.txt could be removed.

We'd still have some code duplication, though, because the non-Apple code path has its own filtering that does mostly the same thing as filter_builtin_sources. The non-Apple code path prints a message for each arch-specific file that overrides a generic file, even if the generic file is already filtered from ${arch}_SOURCES. I could try to deduplicate them.

[compnerd]

If you don't mind, I think that might be an interesting way to try at least. I do see your point about the repetitiveness. I'm not sure if the repeititiveness is worse or the complexity of trying to understand the special function here.

[rprichard]

Split Darwin-specific filtering out of filter_builtin_sources into a
darwin_filter_builtin_sources (which delegates to
filter_builtin_sources), and use a single call to filter_builtin_sources
at the end of CMakeLists.txt to handle the non-Apple code path.

Previously, the filtering at the end of CMakeLists.txt printed a message
for each arch-specific builtin file, but filter_builtin_sources did not
print this message. I chose to keep the message, but I'm not really sure
if we need it? (Maybe it's useful for auditing?) The Darwin build
previously didn't output these messages, and when I tested a Darwin
build, I saw 180 lines. (The Darwin build built several architectures
and several configurations per architecture. I gave each config a
different name so the lines can be distinguished.)

[compnerd]

Thank you for the refactoring here.

[alanphipps]

This comment was removed by alanphipps.

compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake

install(FILES ${LIB_OUTPUT_DIR}/lib${name}.a
       DESTINATION ${LIB_INSTALL_DIR}
       COMPONENT ${install_component})
     add_compiler_rt_install_targets(${name} PARENT_TARGET ${LIB_PARENT_TARGET})
   else()
     message(WARNING "Not generating lipo target for ${name} because no input libraries exist.")
   endif()
 endfunction()
 
+# Filter the list of builtin sources for Darwin, then delegate to the generic
+# filtering.
+#
+# `exclude_or_include` must be one of:
+#  - EXCLUDE: remove every item whose name (w/o extension) matches a name in
+#    `excluded_list`.
+#  - INCLUDE: keep only items whose name (w/o extension) matches something
+#    in `excluded_list`.
+function(darwin_filter_builtin_sources output_var name exclude_or_include excluded_list)
+  if(exclude_or_include STREQUAL "EXCLUDE")
+    set(filter_action GREATER)
+    set(filter_value -1)
+  elseif(exclude_or_include STREQUAL "INCLUDE")
+    set(filter_action LESS)
+    set(filter_value 0)
+  else()
+    message(FATAL_ERROR "darwin_filter_builtin_sources called without EXCLUDE|INCLUDE")
+  endif()
+
+  set(intermediate ${ARGN})
+  foreach(_file ${intermediate})
+    get_filename_component(_name_we ${_file} NAME_WE)
+    list(FIND ${excluded_list} ${_name_we} _found)
+    if(_found ${filter_action} ${filter_value})
+      list(REMOVE_ITEM intermediate ${_file})
+    endif()
+  endforeach()
+
+  filter_builtin_sources(intermediate ${name})
+  set(${output_var} ${intermediate} PARENT_SCOPE)
+endfunction()
+
 # Generates builtin libraries for all operating systems specified in ARGN. Each
 # OS library is constructed by lipo-ing together single-architecture libraries.
 macro(darwin_add_builtin_libraries)
   set(DARWIN_EXCLUDE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/Darwin-excludes)
 
   set(CFLAGS "-fPIC -O3 -fvisibility=hidden -DVISIBILITY_HIDDEN -Wall -fomit-frame-pointer")
   set(CMAKE_C_FLAGS "")
   set(CMAKE_CXX_FLAGS "")
   set(CMAKE_ASM_FLAGS "")
 
   set(PROFILE_SOURCES ../profile/InstrProfiling 
                       ../profile/InstrProfilingBuffer
                       ../profile/InstrProfilingPlatformDarwin
                       ../profile/InstrProfilingWriter)
   foreach (os ${ARGN})
     list_intersect(DARWIN_BUILTIN_ARCHS DARWIN_${os}_BUILTIN_ARCHS BUILTIN_SUPPORTED_ARCH)
     foreach (arch ${DARWIN_BUILTIN_ARCHS})
       darwin_find_excluded_builtins_list(${arch}_${os}_EXCLUDED_BUILTINS
                               OS ${os}
                               ARCH ${arch}
                               MIN_VERSION ${DARWIN_${os}_BUILTIN_MIN_VER})
 
-      filter_builtin_sources(filtered_sources
+      darwin_filter_builtin_sources(filtered_sources
+        ${os}_${arch}
         EXCLUDE ${arch}_${os}_EXCLUDED_BUILTINS
         ${${arch}_SOURCES})
 
       darwin_add_builtin_library(clang_rt builtins
                               OS ${os}
                               ARCH ${arch}
                               SOURCES ${filtered_sources}
                               CFLAGS ${CFLAGS} -arch ${arch}
                               PARENT_TARGET builtins)
     endforeach()
 
     # Don't build cc_kext libraries for simulator platforms
     if(NOT DARWIN_${os}_SKIP_CC_KEXT)
       foreach (arch ${DARWIN_BUILTIN_ARCHS})
         # By not specifying MIN_VERSION this only reads the OS and OS-arch lists.
         # We don't want to filter out the builtins that are present in libSystem
         # because kexts can't link libSystem.
         darwin_find_excluded_builtins_list(${arch}_${os}_EXCLUDED_BUILTINS
                               OS ${os}
                               ARCH ${arch})
 
-        filter_builtin_sources(filtered_sources
+        darwin_filter_builtin_sources(filtered_sources
+          cc_kext_${os}_${arch}
           EXCLUDE ${arch}_${os}_EXCLUDED_BUILTINS
           ${${arch}_SOURCES})
 
         # In addition to the builtins cc_kext includes some profile sources
         darwin_add_builtin_library(clang_rt cc_kext
                                 OS ${os}
                                 ARCH ${arch}
                                 SOURCES ${filtered_sources} ${PROFILE_SOURCES}
if(COMPILER_RT_SUPPORTED_ARCH MATCHES ".*armv.*")
     set(armv6m_FUNCTIONS ${common_FUNCTIONS} ${arm_FUNCTIONS})
     set(armv7m_FUNCTIONS ${common_FUNCTIONS} ${arm_FUNCTIONS} ${thumb2_FUNCTIONS})
     set(armv7em_FUNCTIONS ${common_FUNCTIONS} ${arm_FUNCTIONS} ${thumb2_FUNCTIONS})
     set(armv7_FUNCTIONS ${common_FUNCTIONS} ${arm_FUNCTIONS} ${thumb2_FUNCTIONS} ${thumb2_64_FUNCTIONS})
     set(i386_FUNCTIONS ${common_FUNCTIONS} ${i386_FUNCTIONS})
     set(x86_64_FUNCTIONS ${common_FUNCTIONS})
 
     foreach(arch ${DARWIN_macho_embedded_ARCHS})
-      filter_builtin_sources(${arch}_filtered_sources
+      darwin_filter_builtin_sources(${arch}_filtered_sources
+        macho_embedded_${arch}
         INCLUDE ${arch}_FUNCTIONS
         ${${arch}_SOURCES})
       if(NOT ${arch}_filtered_sources)
         message(WARNING "${arch}_SOURCES: ${${arch}_SOURCES}")
         message(WARNING "${arch}_FUNCTIONS: ${${arch}_FUNCTIONS}")
         message(FATAL_ERROR "Empty filtered sources!")
       endif()
     endforeach()

compiler-rt/cmake/Modules/CompilerRTUtils.cmake

macro(construct_compiler_rt_default_triple)
   # default.
   if(NOT COMPILER_RT_DEFAULT_TARGET_TRIPLE STREQUAL TARGET_TRIPLE)
     set(COMPILER_RT_HAS_EXPLICIT_DEFAULT_TARGET_TRIPLE TRUE)
   else()
     set(COMPILER_RT_HAS_EXPLICIT_DEFAULT_TARGET_TRIPLE FALSE)
   endif()
 endmacro()
 
-# Filter out generic versions of routines that are re-implemented in
-# architecture specific manner.  This prevents multiple definitions of the
-# same symbols, making the symbol selection non-deterministic.
-function(filter_builtin_sources output_var exclude_or_include excluded_list)
-  if(exclude_or_include STREQUAL "EXCLUDE")
-    set(filter_action GREATER)
-    set(filter_value -1)
-  elseif(exclude_or_include STREQUAL "INCLUDE")
-    set(filter_action LESS)
-    set(filter_value 0)
-  else()
-    message(FATAL_ERROR "filter_builtin_sources called without EXCLUDE|INCLUDE")
-  endif()
-
-  set(intermediate ${ARGN})
-  foreach (_file ${intermediate})
-    get_filename_component(_name_we ${_file} NAME_WE)
-    list(FIND ${excluded_list} ${_name_we} _found)
-    if(_found ${filter_action} ${filter_value})
-      list(REMOVE_ITEM intermediate ${_file})
-    elseif(${_file} MATCHES ".*/.*\\.S" OR ${_file} MATCHES ".*/.*\\.c")
+# Filter out generic versions of routines that are re-implemented in an
+# architecture specific manner. This prevents multiple definitions of the same
+# symbols, making the symbol selection non-deterministic.
+#
+# We follow the convention that a source file that exists in a sub-directory
+# (e.g. `ppc/divtc3.c`) is architecture-specific and that if a generic
+# implementation exists it will be a top-level source file with the same name
+# modulo the file extension (e.g. `divtc3.c`).
+function(filter_builtin_sources inout_var name)
+  set(intermediate ${${inout_var}})
+  foreach(_file ${intermediate})
+    get_filename_component(_file_dir ${_file} DIRECTORY)
+    if (NOT "${_file_dir}" STREQUAL "")
+      # Architecture specific file. If a generic version exists, print a notice
+      # and ensure that it is removed from the file list.
       get_filename_component(_name ${_file} NAME)
-      string(REPLACE ".S" ".c" _cname "${_name}")
+      string(REGEX REPLACE "\\.S$" ".c" _cname "${_name}")
+      if(EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/${_cname}")
+        message(STATUS "For ${name} builtins preferring ${_file} to ${_cname}")
         list(REMOVE_ITEM intermediate ${_cname})
-    endif ()
-  endforeach ()
-  set(${output_var} ${intermediate} PARENT_SCOPE)
+      endif()
+    endif()
+  endforeach()
+  set(${inout_var} ${intermediate} PARENT_SCOPE)
 endfunction()
 
 function(get_compiler_rt_target arch variable)
   string(FIND ${COMPILER_RT_DEFAULT_TARGET_TRIPLE} "-" dash_index)
   string(SUBSTRING ${COMPILER_RT_DEFAULT_TARGET_TRIPLE} ${dash_index} -1 triple_suffix)
   if(COMPILER_RT_DEFAULT_TARGET_ONLY)
     # Use exact spelling when building only for the target specified to CMake.
     set(target "${COMPILER_RT_DEFAULT_TARGET_TRIPLE}")

compiler-rt/lib/builtins/CMakeLists.txt

if (NOT MSVC)
   set(x86_ARCH_SOURCES
     ${x86_ARCH_SOURCES}
     i386/fp_mode.c
   )
 endif ()
 
 if (NOT MSVC)
   set(x86_64_SOURCES
+    ${GENERIC_SOURCES}
     ${GENERIC_TF_SOURCES}
+    ${x86_ARCH_SOURCES}
     x86_64/floatdidf.c
     x86_64/floatdisf.c
     x86_64/floatdixf.c
     x86_64/floatundidf.S
     x86_64/floatundisf.S
     x86_64/floatundixf.S
   )
-  filter_builtin_sources(x86_64_SOURCES EXCLUDE x86_64_SOURCES "${x86_64_SOURCES};${GENERIC_SOURCES}")
+
+  # Darwin x86_64 Haswell
   set(x86_64h_SOURCES ${x86_64_SOURCES})
 
   if (WIN32)
     set(x86_64_SOURCES
       ${x86_64_SOURCES}
       x86_64/chkstk.S
       x86_64/chkstk2.S
     )
   endif()
 
   set(i386_SOURCES
+    ${GENERIC_SOURCES}
+    ${x86_ARCH_SOURCES}
     i386/ashldi3.S
     i386/ashrdi3.S
     i386/divdi3.S
     i386/floatdidf.S
     i386/floatdisf.S
     i386/floatdixf.S
     i386/floatundidf.S
     i386/floatundisf.S
     i386/floatundixf.S
     i386/lshrdi3.S
     i386/moddi3.S
     i386/muldi3.S
     i386/udivdi3.S
     i386/umoddi3.S
   )
-  filter_builtin_sources(i386_SOURCES EXCLUDE i386_SOURCES "${i386_SOURCES};${GENERIC_SOURCES}")
 
   if (WIN32)
     set(i386_SOURCES
       ${i386_SOURCES}
       i386/chkstk.S
       i386/chkstk2.S
     )
   endif()
 else () # MSVC
   # Use C versions of functions when building on MSVC
   # MSVC's assembler takes Intel syntax, not AT&T syntax.
   # Also use only MSVC compilable builtin implementations.
   set(x86_64_SOURCES
+    ${GENERIC_SOURCES}
+    ${x86_ARCH_SOURCES}
     x86_64/floatdidf.c
     x86_64/floatdisf.c
     x86_64/floatdixf.c
-    ${GENERIC_SOURCES}
   )
-  set(x86_64h_SOURCES ${x86_64_SOURCES})
-  set(i386_SOURCES ${GENERIC_SOURCES})
+  set(i386_SOURCES ${GENERIC_SOURCES} ${x86_ARCH_SOURCES})
 endif () # if (NOT MSVC)
 
-set(x86_64h_SOURCES ${x86_64h_SOURCES} ${x86_ARCH_SOURCES})
-set(x86_64_SOURCES ${x86_64_SOURCES} ${x86_ARCH_SOURCES})
-set(i386_SOURCES ${i386_SOURCES} ${x86_ARCH_SOURCES})
-set(i686_SOURCES ${i686_SOURCES} ${x86_ARCH_SOURCES})
-
 set(arm_SOURCES
   arm/fp_mode.c
   arm/bswapdi2.S
   arm/bswapsi2.S
   arm/clzdi2.S
   arm/clzsi2.S
   arm/comparesf2.S
   arm/divmodsi4.S
set(arm_SOURCES
   arm/sync_fetch_and_umax_8.S
   arm/sync_fetch_and_umin_4.S
   arm/sync_fetch_and_umin_8.S
   arm/sync_fetch_and_xor_4.S
   arm/sync_fetch_and_xor_8.S
   arm/udivmodsi4.S
   arm/udivsi3.S
   arm/umodsi3.S
+  ${GENERIC_SOURCES}
 )
-filter_builtin_sources(arm_SOURCES EXCLUDE arm_SOURCES "${arm_SOURCES};${GENERIC_SOURCES}")
 
 set(thumb1_SOURCES
   arm/divsi3.S
   arm/udivsi3.S
   arm/comparesf2.S
   arm/addsf3.S
   ${GENERIC_SOURCES}
 )

 if(MINGW)
   set(arm_SOURCES
     arm/aeabi_idivmod.S
     arm/aeabi_ldivmod.S
     arm/aeabi_uidivmod.S
     arm/aeabi_uldivmod.S
     arm/chkstk.S
     mingw_fixfloat.c
+    ${GENERIC_SOURCES}
   )
-  filter_builtin_sources(arm_SOURCES EXCLUDE arm_SOURCES "${arm_SOURCES};${GENERIC_SOURCES}")
 elseif(NOT WIN32)
   # TODO the EABI sources should only be added to EABI targets
   set(arm_SOURCES
     ${arm_SOURCES}
     ${arm_EABI_SOURCES}
     ${arm_Thumb1_SOURCES}
   )
 
if (CAN_TARGET_${arch})
       if (${arch} MATCHES "^(arm|armhf|armv7|armv7s|armv7k|armv7m|armv7em)$")
         string(REPLACE ";" " " _TARGET_${arch}_CFLAGS "${TARGET_${arch}_CFLAGS}")
         check_compile_definition(__VFP_FP__ "${CMAKE_C_FLAGS} ${_TARGET_${arch}_CFLAGS}" COMPILER_RT_HAS_${arch}_VFP)
         if(NOT COMPILER_RT_HAS_${arch}_VFP)
           list(REMOVE_ITEM ${arch}_SOURCES ${arm_Thumb1_VFPv2_SOURCES} ${arm_Thumb1_SjLj_EH_SOURCES})
         endif()
       endif()
 
-      # Filter out generic versions of routines that are re-implemented in
-      # architecture specific manner.  This prevents multiple definitions of the
-      # same symbols, making the symbol selection non-deterministic.
-      foreach (_file ${${arch}_SOURCES})
-        get_filename_component(_file_dir "${_file}" DIRECTORY)
-        if (NOT "${_file_dir}" STREQUAL "")
-          # Architecture specific file. We follow the convention that a source
-          # file that exists in a sub-directory (e.g. `ppc/divtc3.c`) is
-          # architecture specific and that if a generic implementation exists
-          # it will be a top-level source file with the same name modulo the
-          # file extension (e.g. `divtc3.c`).
-          get_filename_component(_name ${_file} NAME)
-          string(REPLACE ".S" ".c" _cname "${_name}")
-          if (EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/${_cname}")
-            message(STATUS "For ${arch} builtins preferring ${_file} to ${_cname}")
-            list(REMOVE_ITEM ${arch}_SOURCES ${_cname})
-          endif()
-        endif ()
-      endforeach ()
+      # Remove a generic C builtin when an arch-specific builtin is specified.
+      filter_builtin_sources(${arch}_SOURCES ${arch})
 
       # Needed for clear_cache on debug mode, due to r7's usage in inline asm.
       # Release mode already sets it via -O2/3, Debug mode doesn't.
       if (${arch} STREQUAL "armhf")
         list(APPEND BUILTIN_CFLAGS -fomit-frame-pointer -DCOMPILER_RT_ARMHF_TARGET)
       endif()
 
       # For RISCV32, we must force enable int128 for compiling long