Rdar 29016063 precompile bridging header #5977

graydon · 2016-11-30T01:22:18Z

This is a preliminary implementation of PCH-for-bridging-headers; the point is to accelerate non-WMO builds with large bridging headers, which spend a nontrivial amount of time re-importing the header for each frontend job.

Caveats:

Not sure the implementation strategy is really what anyone wanted
Probably fails to handle a bunch of invalid args / errors / invalid file situations
SourceKit and any other clients that want access to source info for the bridging header are neglected
No confirmation yet that it's any faster for the cases we're concerned about
Inadequate tests

Still, I'm posting for early feedback since it's past the point of "working" and seems tidy / non-crashy enough to play with / evaluate.

rdar://problem/29016063

graydon · 2016-11-30T01:23:43Z

@swift-ci please smoke test

jrose-apple

Many comments but overall it all makes sense. Nice work, Graydon!

jrose-apple · 2017-01-04T21:40:19Z

include/swift/Option/Options.td

@@ -417,6 +417,10 @@ def emit_sib : Flag<["-"], "emit-sib">,
 def emit_sibgen : Flag<["-"], "emit-sibgen">,
  HelpText<"Emit serialized AST + raw SIL file(s)">, ModeOpt,
  Flags<[FrontendOption, NoInteractiveOption, DoesNotAffectIncrementalBuild]>;
+def emit_pch : Flag<["-"], "emit-pch">,


I'm not sure we need to bother making this a driver option at all. It's easy enough to test invoking the frontend directly, and no outside tool should need to rely on doing it manually.

jrose-apple · 2017-01-04T21:41:40Z

lib/Frontend/CompilerInvocation.cpp

@@ -482,6 +484,14 @@ static bool ParseFrontendArgs(FrontendOptions &Opts, ArgList &Args,
      Opts.setSingleOutputFilename("-");
      break;

+    case FrontendOptions::EmitPCH: {
+      if (Opts.OutputFilenames.empty())
+        Opts.setSingleOutputFilename("-");


PCH is a non-textual format, so it shouldn't default to stdout.

jrose-apple · 2017-01-04T21:45:08Z

lib/Frontend/Frontend.cpp

@@ -311,7 +311,13 @@ void CompilerInstance::performSema() {
  Module *importedHeaderModule = nullptr;
  StringRef implicitHeaderPath = options.ImplicitObjCHeaderPath;
  if (!implicitHeaderPath.empty()) {
-    if (!clangImporter->importBridgingHeader(implicitHeaderPath, MainModule)) {
+    bool failed = false;
+    if (llvm::sys::path::extension(implicitHeaderPath).endswith(PCH_EXTENSION)) {


I think I would sink this logic inside importBridgingHeader. The less the frontend knows about gory Clang details the better.

jrose-apple · 2017-01-04T21:46:35Z

include/swift/Option/Options.td

@@ -237,6 +237,10 @@ def disable_swift_bridge_attr : Flag<["-"], "disable-swift-bridge-attr">,
  Flags<[FrontendOption, HelpHidden]>,
  HelpText<"Disable using the swift bridge attribute">;

+def disable_bridging_pch : Flag<["-"], "disable-bridging-pch">,
+  Flags<[FrontendOption, HelpHidden]>,


Not a FrontendOption. :-)

jrose-apple · 2017-01-04T21:47:19Z

lib/Driver/Driver.cpp

+          // JobActions so they don't try to free it. That in turn means we need
+          // to transfer ownership of all the JobActions' existing inputs to the
+          // Actions array, since the JobActions either own or don't own _all_
+          // of their inputs. Ownership can't vary input-by-input.


Can you tag this with a FIXME? I want to go back and clear up the Action ownership muck at some point.

jrose-apple · 2017-01-04T21:52:40Z

lib/Driver/ToolChains.cpp

+                A->getValue()) {
+              Arg Tmp(A->getOption(), A->getSpelling(), A->getIndex(),
+                      IJ->getOutput().getPrimaryOutputFilename().c_str(), A);
+              Tmp.render(context.Args, Arguments);


Just doing this manually is fine.

Arguments.push_back("-import-objc-header"); Arguments.push_Back(IJ->getOutput().getPrimaryOutputFilename().c_str());

jrose-apple · 2017-01-04T21:54:42Z

test/ClangImporter/pch-bridging-header.swift

+// REQUIRES: objc_interop
+// RUN: rm -rf %t && mkdir -p %t/tmp
+
+// First test the explicit frontend-based bridging PCH generation and use works


Please add tests under test/Driver/ using -driver-print-actions and -driver-print-jobs, so that we can see the individual steps being generated properly.

jrose-apple · 2017-01-04T21:55:41Z

include/swift/AST/DiagnosticsClangImporter.def

@@ -79,6 +79,11 @@ WARNING(unresolvable_clang_decl,none,
        "imported declaration '%0' could not be mapped to '%1'",
        (StringRef, StringRef))

+WARNING(implicit_bridging_header_imported_from_module,none,
+        "implicit import of bridging header '%0' via module '%1' "
+        "is deprecated, will be removed in a later version of Swift",


Grammar nitpick: let's go with and instead of the comma.

jrose-apple · 2017-01-04T21:57:34Z

lib/ClangImporter/ClangImporter.cpp

+  if (implicitImport && !allParsedDecls.empty()) {
+    SwiftContext.Diags.diagnose(
+      diagLoc, diag::implicit_bridging_header_imported_from_module,
+      headerName, adapter->getNameStr());


Maybe we should just print the last path component of the header here.

Instead of using getNameStr(), you could change the diagnostic to take an Identifier.

jrose-apple · 2017-01-04T21:58:55Z

test/ClangImporter/MixedSource/import-mixed-with-header.swift

@@ -14,7 +14,7 @@

 // XFAIL: linux

-import MixedWithHeader
+import MixedWithHeader // expected-warning {{implicit import of bridging header}}


Please check the entire diagnostic in at least one of these tests, even if you have to use FileCheck to do it (to avoid embedding a full path).

graydon · 2017-01-05T04:16:09Z

Updated to address all review comments. Much improved, thanks!

graydon · 2017-01-05T05:03:11Z

@swift-ci please smoke test

jrose-apple

This looks great, Graydon. Nice work!

jrose-apple · 2017-01-05T19:32:27Z

lib/Driver/ToolChains.cpp

+    for (auto *IJ : context.Inputs) {
+      if (!IJ->getOutput().getAnyOutputForType(types::TY_PCH).empty()) {
+        Arguments.push_back("-import-objc-header");
+        addInputsOfType(Arguments, context.Inputs, types::TY_PCH);


It does bug me a little that we end up searching the input list twice here, but I guess it will usually be short. You could also have addInputsOfType return a boolean or a count, though (and explicitly get the value out of the argument in the non-PCH case, rather than forwarding with AddLastArg).

modocache · 2017-01-10T16:15:03Z

include/swift/AST/DiagnosticsClangImporter.def

@@ -53,6 +53,9 @@ ERROR(bridging_header_error,Fatal,
 WARNING(could_not_rewrite_bridging_header,none,
  "failed to serialize bridging header; "
  "target may not be debuggable outside of its original project", ())
+ERROR(bridging_header_pch_error,Fatal,
+   "failed to emit PCH file '%0' for bridging header '%1'",


Would it make sense to write "precompiled header" here, instead of "PCH"? There's a chance end users could see this error, and they might not be familiar with the acronym.

modocache · 2017-01-10T16:19:27Z

include/swift/ClangImporter/ClangImporter.h

@@ -218,6 +223,14 @@ class ClangImporter final : public ClangModuleLoader {
  std::string getBridgingHeaderContents(StringRef headerPath, off_t &fileSize,
                                        time_t &fileModTime);

+  /// Makes a temporary replica of the ClangImporter's CompilerInstance, reads
+  /// an Objective-C header file into the replica and emits a PCH file of its


These docblocks are fantastic!

nit-pick: Sorry to harp on about acronyms, but I noticed "PCH" isn't in docs/Lexicon.rst, either. I think that some Swift contributors might not be immediately familiar with the concept of a precompiled header. Since this pull request is the first use of "PCH" in a docblock in this codebase, I'd suggest adding the acronym to the Lexicon as well.

jrose-apple · 2017-01-10T16:59:35Z

lib/ClangImporter/ClangImporter.cpp

-  if (llvm::sys::path::extension(importerOpts.BridgingHeader).endswith(
-        PCH_EXTENSION))
-    clangPP.setPredefines("");
+  // FIXME: This is missing implicit includes.


Is this FIXME still true? This is about there being -include options passed via -Xcc and the importer not exposing the contents.

jrose-apple · 2017-01-10T17:00:31Z

lib/ClangImporter/ClangImporter.cpp

+    // processing.
+    for (auto CM : Impl.PCHReadModules) {
+      auto L = nextSyntheticImportLocation();
+      getClangPreprocessor().makeModuleVisible(CM, L);


Shouldn't it already be visible thanks to the PCH? You may be able to get away with just calling finishLoadingClangModule.

jrose-apple · 2017-01-10T17:01:37Z

lib/ClangImporter/ImporterImpl.h

@@ -272,6 +272,9 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
  const bool InferImportAsMember;
  const bool DisableSwiftBridgeAttr;

+  bool IsReadingBridgingPCH;
+  llvm::DenseSet<clang::Module *> PCHReadModules;


Please use a SetVector or even a regular SmallVector here. Order probably matters for determinism in diagnostics, if not normal name lookup.

ematejska · 2017-01-13T20:12:48Z

@swift-ci test

swift-ci · 2017-01-13T20:13:08Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - ef4a7bf04f7bd36b89872506221caa1b23b2bdd1
Test requested by - @ematejska

swift-ci · 2017-01-13T20:13:17Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - ef4a7bf04f7bd36b89872506221caa1b23b2bdd1
Test requested by - @ematejska

…tly.

…ule.

…ver.

We're trying to get rid of implicit bridging-header imports, as a feature. These are IMPORTED_HEADER blocks left in modules built with bridging headers, that trigger re-importing the bridging header into any client that imports the module. As a half-way measure to deprecating them, we add a warning here that triggers when an implicit bridging-header import occurs that is _not_ suppressed as redundant by clang.

This happens fairly regularly in unit testing scenarios.

graydon · 2017-01-14T03:49:27Z

@swift-ci please test and merge

graydon force-pushed the rdar-29016063-precompile-bridging-header branch from 88a64a3 to f390300 Compare November 30, 2016 01:31

graydon force-pushed the rdar-29016063-precompile-bridging-header branch from f390300 to da9dee8 Compare December 8, 2016 17:43

graydon force-pushed the rdar-29016063-precompile-bridging-header branch 3 times, most recently from a43605f to ffc97d7 Compare December 22, 2016 06:49

graydon force-pushed the rdar-29016063-precompile-bridging-header branch from ffc97d7 to 1163e7f Compare January 4, 2017 21:07

jrose-apple reviewed Jan 4, 2017

View reviewed changes

graydon force-pushed the rdar-29016063-precompile-bridging-header branch from 1163e7f to ef4a7bf Compare January 5, 2017 04:15

jrose-apple approved these changes Jan 5, 2017

View reviewed changes

graydon force-pushed the rdar-29016063-precompile-bridging-header branch 2 times, most recently from c072691 to 6666f98 Compare January 6, 2017 21:21

modocache reviewed Jan 10, 2017

View reviewed changes

jrose-apple reviewed Jan 10, 2017

View reviewed changes

graydon force-pushed the rdar-29016063-precompile-bridging-header branch from 9ea6133 to 058eafc Compare January 11, 2017 19:31

benasher44 mentioned this pull request Jan 12, 2017

[52] Issue #52 - January 12, 2017 SwiftWeekly/swiftweekly.github.io#159

Closed

graydon force-pushed the rdar-29016063-precompile-bridging-header branch 2 times, most recently from 8c6d1ec to 9b2912e Compare January 13, 2017 00:58

graydon mentioned this pull request Jan 13, 2017

Rdar 29016063 swift 3.1 branch precompile bridging header #6768

Merged

graydon added 6 commits January 13, 2017 15:18

[Bridging PCH] Add ClangImporter::emitBridgingPCH.

01a7f46

[Bridging PCH] Add -emit-pch to Frontend; call emitBridgingPCH.

a95f5da

[Bridging PCH] Store ClangImporter::BridgingHeaderLookupTable indirec…

2b638dc

…tly.

[Bridging PCH] Populate bridging lookup table when reading MK_PCH mod…

d709aff

…ule.

[Bridging PCH] Pass .pch bridging headers to clang -import-pch

874dc3b

[Bridging PCH] Add testcase for emitting, importing a bridging PCH file.

75d4b03

graydon added 5 commits January 13, 2017 15:18

[Bridging PCH] Auto-generate and use temporary bridging PCHs from dri…

3b0b8c3

…ver.

[Bridging PCH] Handle mixed PCH + implicit bridging header imports.

52db044

This happens fairly regularly in unit testing scenarios.

[Bridging PCH] Add -enable-bridging-pch option, set default to disabled.

e1b0cc9

[Bridging PCH] Terminology clarification as suggested in review.

5f747ea

graydon force-pushed the rdar-29016063-precompile-bridging-header branch from 9b2912e to 5f747ea Compare January 14, 2017 03:46

graydon changed the title ~~(Don't merge yet) Rdar 29016063 precompile bridging header~~ Rdar 29016063 precompile bridging header Jan 14, 2017

graydon merged commit 28f1248 into swiftlang:master Jan 14, 2017

graydon deleted the rdar-29016063-precompile-bridging-header branch September 13, 2017 16:42

polac24 mentioned this pull request Aug 25, 2022

Precompile prefix headers fails on Consumer because the -Swift.h file includes an absolute path from Producer spotify/XCRemoteCache#140

Open

Rdar 29016063 precompile bridging header #5977

Rdar 29016063 precompile bridging header #5977

Uh oh!

Conversation

graydon commented Nov 30, 2016 • edited by amartini51 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graydon commented Nov 30, 2016

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graydon commented Jan 5, 2017

Uh oh!

graydon commented Jan 5, 2017

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ematejska commented Jan 13, 2017

Uh oh!

swift-ci commented Jan 13, 2017

Uh oh!

swift-ci commented Jan 13, 2017

Uh oh!

graydon commented Jan 14, 2017

Uh oh!

Uh oh!

graydon commented Nov 30, 2016 •

edited by amartini51

Loading