[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/lib/builtins/CMakeLists.txt

if (COMPILER_RT_STANDALONE_BUILD)
   # LLVM configuration is not being utilized.
   option(COMPILER_RT_BUILTINS_ENABLE_PIC
     "Turns on or off -fPIC for the builtin library source"
     ON)
 endif()
 
 include(builtin-config-ix)
 
+# Pass an empty list to filter_builtin_sources to disable part of its filtering
+# logic.
+set(NOTHING_TO_EXCLUDE "")
+
 # TODO: Need to add a mechanism for logging errors when builtin source files are
 # added to a sub-directory and not this CMakeLists file.
 set(GENERIC_SOURCES
   absvdi2.c
   absvsi2.c
   absvti2.c
   adddf3.c
   addsf3.c
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})
+  filter_builtin_sources(x86_64h_SOURCES EXCLUDE NOTHING_TO_EXCLUDE "${x86_64h_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})
+# TODO: These filter_builtin_sources calls seem redundant with the
+# generic-vs-arch-specific handling at the end of this file.
+filter_builtin_sources(x86_64_SOURCES EXCLUDE NOTHING_TO_EXCLUDE "${x86_64_SOURCES}")
+filter_builtin_sources(i386_SOURCES EXCLUDE NOTHING_TO_EXCLUDE "${i386_SOURCES}")
 
 set(arm_SOURCES
   arm/fp_mode.c
   arm/bswapdi2.S
   arm/bswapsi2.S
   arm/clzdi2.S
   arm/clzsi2.S
   arm/comparesf2.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}")
+filter_builtin_sources(arm_SOURCES EXCLUDE NOTHING_TO_EXCLUDE "${arm_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}")
+  filter_builtin_sources(arm_SOURCES EXCLUDE NOTHING_TO_EXCLUDE "${arm_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}
   )