Page MenuHomePhabricator

[builtins] Cleanup generic-file filtering
ClosedPublic

Authored by rprichard on Jun 18 2020, 11:59 PM.

Details

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.

Diff Detail

Event Timeline

rprichard created this revision.Jun 18 2020, 11:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 18 2020, 11:59 PM
Herald added subscribers: Restricted Project, mgorny. · View Herald Transcript

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.

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

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.

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.

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.

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

rprichard retitled this revision from [builtins] Cleanup generic-file filtering a bit to [builtins] Cleanup generic-file filtering.Jul 10 2020, 12:01 AM
rprichard edited the summary of this revision. (Show Details)
compnerd accepted this revision.Jul 10 2020, 9:50 AM

Thank you for the refactoring here.

This revision is now accepted and ready to land.Jul 10 2020, 9:50 AM
This revision was automatically updated to reflect the committed changes.
This comment was removed by alanphipps.