Merge pull request #34231 from CodaFi/skippy

Skip Merge-Modules When We Can

include/swift/Driver/Action.h

@@ -130,15 +130,28 @@ public:
 class IncrementalJobAction : public JobAction {
 public:
   struct InputInfo {
-    enum Status {
+    /// The status of an input known to the driver. These are used to affect
+    /// the scheduling decisions made during an incremental build.
+    ///
+    /// \Note The order of cases matters. They are ordered from least to
+    /// greatest impact on the incremental build schedule.
+    enum class Status {
+      /// The input to this job is up to date.
       UpToDate,
-      NeedsCascadingBuild,
+      /// The input to this job has changed in a way that requires this job to
+      /// be rerun, but not in such a way that it requires a cascading rebuild.
       NeedsNonCascadingBuild,
+      /// The input to this job has changed in a way that requires this job to
+      /// be rerun, and in such a way that all jobs dependent upon this one
+      /// must be scheduled as well.
+      NeedsCascadingBuild,
+      /// The input to this job was not known to the driver when it was last
+      /// run.
       NewlyAdded
     };
 
   public:
-    Status status = UpToDate;
+    Status status = Status::UpToDate;
     llvm::sys::TimePoint<> previousModTime;
 
     InputInfo() = default;
@@ -146,7 +159,11 @@ public:
         : status(stat), previousModTime(time) {}
 
     static InputInfo makeNewlyAdded() {
-      return InputInfo(Status::NewlyAdded, llvm::sys::TimePoint<>::max());
+      return {Status::NewlyAdded, llvm::sys::TimePoint<>::max()};
+    }
+
+    static InputInfo makeNeedsCascadingRebuild() {
+      return {Status::NeedsCascadingBuild, llvm::sys::TimePoint<>::min()};
     }
   };
 
@@ -166,7 +183,8 @@ public:
 
 public:
   static bool classof(const Action *A) {
-    return A->getKind() == Action::Kind::CompileJob;
+    return A->getKind() == Action::Kind::CompileJob ||
+           A->getKind() == Action::Kind::MergeModuleJob;
   }
 };
 
@@ -263,12 +281,12 @@ public:
   }
 };
 
-class MergeModuleJobAction : public JobAction {
+class MergeModuleJobAction : public IncrementalJobAction {
   virtual void anchor() override;
 public:
-  MergeModuleJobAction(ArrayRef<const Action *> Inputs)
+  MergeModuleJobAction(ArrayRef<const Action *> Inputs, InputInfo input)
-      : JobAction(Action::Kind::MergeModuleJob, Inputs,
+      : IncrementalJobAction(Action::Kind::MergeModuleJob, Inputs,
-                  file_types::TY_SwiftModuleFile) {}
+                             file_types::TY_SwiftModuleFile, input) {}
 
   static bool classof(const Action *A) {
     return A->getKind() == Action::Kind::MergeModuleJob;

lib/Driver/Compilation.cpp

@@ -446,7 +446,7 @@ namespace driver {
 
     std::vector<const Job*>
     reloadAndRemarkDeps(const Job *FinishedCmd, int ReturnCode,
-                             const bool forRanges) {
+                        const bool forRanges) {
       const CommandOutput &Output = FinishedCmd->getOutput();
       StringRef DependenciesFile =
           Output.getAdditionalOutputForType(file_types::TY_SwiftDeps);
@@ -458,7 +458,8 @@ namespace driver {
         // coarse dependencies that always affect downstream nodes), but we're
         // not using either of those right now, and this logic should probably
         // be revisited when we are.
-        assert(FinishedCmd->getCondition() == Job::Condition::Always);
+        assert(isa<MergeModuleJobAction>(FinishedCmd->getSource()) ||
+               FinishedCmd->getCondition() == Job::Condition::Always);
         return {};
       }
       const bool compileExitedNormally =
@@ -907,6 +908,7 @@ namespace driver {
         return everyIncrementalJob;
       };
 
+      const Job *mergeModulesJob = nullptr;
       CommandSet jobsToSchedule;
       CommandSet initialCascadingCommands;
       for (const Job *cmd : Comp.getJobs()) {
@@ -915,6 +917,11 @@ namespace driver {
           continue;
         }
 
+        if (isa<MergeModuleJobAction>(cmd->getSource())) {
+          assert(!mergeModulesJob && "multiple scheduled merge-modules jobs?");
+          mergeModulesJob = cmd;
+        }
+
         const Optional<std::pair<bool, bool>> shouldSchedAndIsCascading =
             computeShouldInitiallyScheduleJobAndDependendents(cmd, forRanges);
         if (!shouldSchedAndIsCascading)
@@ -936,6 +943,15 @@ namespace driver {
            collectIncrementalExternallyDependentJobsFromDependencyGraph(
                forRanges))
         jobsToSchedule.insert(cmd);
+
+      // The merge-modules job is special: it *must* be scheduled if any other
+      // job has been scheduled because any other job can influence the
+      // structure of the resulting module. Additionally, the initial scheduling
+      // predicate above is only aware of intra-module changes. External
+      // dependencies changing *must* cause merge-modules to be scheduled.
+      if (!jobsToSchedule.empty() && mergeModulesJob) {
+        jobsToSchedule.insert(mergeModulesJob);
+      }
       return jobsToSchedule;
     }
 
@@ -1031,6 +1047,13 @@ namespace driver {
     /// But returns None if there was a dependency read error.
     Optional<std::pair<Job::Condition, bool>>
     loadDependenciesAndComputeCondition(const Job *const Cmd, bool forRanges) {
+      // merge-modules Jobs do not have .swiftdeps files associated with them,
+      // however, their compilation condition is computed as a function of their
+      // inputs, so their condition can be used as normal.
+      if (isa<MergeModuleJobAction>(Cmd->getSource())) {
+        return std::make_pair(Cmd->getCondition(), true);
+      }
+
       // Try to load the dependencies file for this job. If there isn't one, we
       // always have to run the job, but it doesn't affect any other jobs. If
       // there should be one but it's not present or can't be loaded, we have to
@@ -1163,7 +1186,12 @@ namespace driver {
           continue;
         }
 
-        // Can we run a cross-module incremental build at all? If not, fallback.
+        // Is this module out of date? If not, just keep searching.
+        if (Comp.getLastBuildTime() >= depStatus.getLastModificationTime())
+          continue;
+
+        // Can we run a cross-module incremental build at all?
+        // If not, fall back.
         if (!Comp.getEnableCrossModuleIncrementalBuild()) {
           fallbackToExternalBehavior(external);
           continue;
@@ -1609,8 +1637,8 @@ namespace driver {
           CompileJobAction::InputInfo info;
           info.previousModTime = entry.first->getInputModTime();
           info.status = entry.second ?
-            CompileJobAction::InputInfo::NeedsCascadingBuild :
+            CompileJobAction::InputInfo::Status::NeedsCascadingBuild :
-            CompileJobAction::InputInfo::NeedsNonCascadingBuild;
+            CompileJobAction::InputInfo::Status::NeedsNonCascadingBuild;
           inputs[&inputFile->getInputArg()] = info;
         }
       }
@@ -1627,7 +1655,7 @@ namespace driver {
 
           CompileJobAction::InputInfo info;
           info.previousModTime = entry->getInputModTime();
-          info.status = CompileJobAction::InputInfo::UpToDate;
+          info.status = CompileJobAction::InputInfo::Status::UpToDate;
           inputs[&inputFile->getInputArg()] = info;
         }
       }

lib/Driver/CompilationRecord.h

@@ -59,12 +59,12 @@ inline static StringRef getName(TopLevelKey Key) {
 inline static StringRef
 getIdentifierForInputInfoStatus(CompileJobAction::InputInfo::Status Status) {
   switch (Status) {
-  case CompileJobAction::InputInfo::UpToDate:
+  case CompileJobAction::InputInfo::Status::UpToDate:
     return "";
-  case CompileJobAction::InputInfo::NewlyAdded:
+  case CompileJobAction::InputInfo::Status::NewlyAdded:
-  case CompileJobAction::InputInfo::NeedsCascadingBuild:
+  case CompileJobAction::InputInfo::Status::NeedsCascadingBuild:
     return "!dirty";
-  case CompileJobAction::InputInfo::NeedsNonCascadingBuild:
+  case CompileJobAction::InputInfo::Status::NeedsNonCascadingBuild:
     return "!private";
   }
 
@@ -76,11 +76,11 @@ getIdentifierForInputInfoStatus(CompileJobAction::InputInfo::Status Status) {
 /// compilation record file (.swiftdeps file).
 inline static Optional<CompileJobAction::InputInfo::Status>
 getInfoStatusForIdentifier(StringRef Identifier) {
-  return llvm::StringSwitch<Optional<
+  using InputStatus = CompileJobAction::InputInfo::Status;
-      CompileJobAction::InputInfo::Status>>(Identifier)
+  return llvm::StringSwitch<Optional<InputStatus>>(Identifier)
-    .Case("", CompileJobAction::InputInfo::UpToDate)
+    .Case("", InputStatus::UpToDate)
-    .Case("!dirty", CompileJobAction::InputInfo::NeedsCascadingBuild)
+    .Case("!dirty", InputStatus::NeedsCascadingBuild)
-    .Case("!private", CompileJobAction::InputInfo::NeedsNonCascadingBuild)
+    .Case("!private", InputStatus::NeedsNonCascadingBuild)
     .Default(None);
 }
 

lib/Driver/Driver.cpp

@@ -1876,6 +1876,54 @@ Driver::computeCompilerMode(const DerivedArgList &Args,
   return OutputInfo::Mode::StandardCompile;
 }
 
+namespace {
+/// Encapsulates the computation of input jobs that are relevant to the
+/// merge-modules job the scheduler can insert if we are not in a single compile
+/// mode.
+class ModuleInputs final {
+private:
+  using InputInfo = IncrementalJobAction::InputInfo;
+  SmallVector<const Action *, 2> AllModuleInputs;
+  InputInfo StatusBound;
+
+public:
+  explicit ModuleInputs()
+      : StatusBound
+            {InputInfo::Status::UpToDate, llvm::sys::TimePoint<>::min()} {}
+
+public:
+  void addInput(const Action *inputAction) {
+    if (auto *IJA = dyn_cast<IncrementalJobAction>(inputAction)) {
+      // Take the upper bound of the status of any incremental inputs to
+      // ensure that the merge-modules job gets run if *any* input job is run.
+      const auto conservativeStatus =
+          std::max(StatusBound.status, IJA->getInputInfo().status);
+      // The modification time here is not important to the rest of the
+      // incremental build. We take the upper bound in case an attempt to
+      // compare the swiftmodule output's mod time and any input files is
+      // made. If the compilation has been correctly scheduled, the
+      // swiftmodule's mod time will always strictly exceed the mod time of
+      // any of its inputs when we are able to skip it.
+      const auto conservativeModTime = std::max(
+          StatusBound.previousModTime, IJA->getInputInfo().previousModTime);
+      StatusBound = InputInfo{conservativeStatus, conservativeModTime};
+    }
+    AllModuleInputs.push_back(inputAction);
+  }
+
+public:
+  /// Returns \c true if no inputs have been registered with this instance.
+  bool empty() const { return AllModuleInputs.empty(); }
+
+public:
+  /// Consumes this \c ModuleInputs instance and returns a merge-modules action
+  /// from the list of input actions and status it has computed thus far.
+  JobAction *intoAction(Compilation &C) && {
+    return C.createAction<MergeModuleJobAction>(AllModuleInputs, StatusBound);
+  }
+};
+} // namespace
+
 void Driver::buildActions(SmallVectorImpl<const Action *> &TopLevelActions,
                           const ToolChain &TC, const OutputInfo &OI,
                           const InputInfoMap *OutOfDateMap,
@@ -1888,7 +1936,7 @@ void Driver::buildActions(SmallVectorImpl<const Action *> &TopLevelActions,
     return;
   }
 
-  SmallVector<const Action *, 2> AllModuleInputs;
+  ModuleInputs AllModuleInputs;
   SmallVector<const Action *, 2> AllLinkerInputs;
 
   switch (OI.CompilerMode) {
@@ -1929,10 +1977,8 @@ void Driver::buildActions(SmallVectorImpl<const Action *> &TopLevelActions,
         // Source inputs always need to be compiled.
         assert(file_types::isPartOfSwiftCompilation(InputType));
 
-        CompileJobAction::InputInfo previousBuildState = {
+        auto previousBuildState =
-          CompileJobAction::InputInfo::NeedsCascadingBuild,
+            IncrementalJobAction::InputInfo::makeNeedsCascadingRebuild();
-          llvm::sys::TimePoint<>::min()
-        };
         if (OutOfDateMap)
           previousBuildState = OutOfDateMap->lookup(InputArg);
         if (Args.hasArg(options::OPT_embed_bitcode)) {
@@ -1940,7 +1986,7 @@ void Driver::buildActions(SmallVectorImpl<const Action *> &TopLevelActions,
               Current, file_types::TY_LLVM_BC, previousBuildState);
           if (PCH)
             cast<JobAction>(Current)->addInput(PCH);
-          AllModuleInputs.push_back(Current);
+          AllModuleInputs.addInput(Current);
           Current = C.createAction<BackendJobAction>(Current,
                                                      OI.CompilerOutputType, 0);
         } else {
@@ -1949,7 +1995,7 @@ void Driver::buildActions(SmallVectorImpl<const Action *> &TopLevelActions,
                                                      previousBuildState);
           if (PCH)
             cast<JobAction>(Current)->addInput(PCH);
-          AllModuleInputs.push_back(Current);
+          AllModuleInputs.addInput(Current);
         }
         AllLinkerInputs.push_back(Current);
         break;
@@ -1961,7 +2007,7 @@ void Driver::buildActions(SmallVectorImpl<const Action *> &TopLevelActions,
           // When generating a .swiftmodule as a top-level output (as opposed
           // to, for example, linking an image), treat .swiftmodule files as
           // inputs to a MergeModule action.
-          AllModuleInputs.push_back(Current);
+          AllModuleInputs.addInput(Current);
           break;
         } else if (OI.shouldLink()) {
           // Otherwise, if linking, pass .swiftmodule files as inputs to the
@@ -2043,7 +2089,7 @@ void Driver::buildActions(SmallVectorImpl<const Action *> &TopLevelActions,
         // Create a single CompileJobAction and a single BackendJobAction.
         JobAction *CA =
             C.createAction<CompileJobAction>(file_types::TY_LLVM_BC);
-        AllModuleInputs.push_back(CA);
+        AllModuleInputs.addInput(CA);
 
         int InputIndex = 0;
         for (const InputPair &Input : Inputs) {
@@ -2079,7 +2125,7 @@ void Driver::buildActions(SmallVectorImpl<const Action *> &TopLevelActions,
 
       CA->addInput(C.createAction<InputAction>(*InputArg, InputType));
     }
-    AllModuleInputs.push_back(CA);
+    AllModuleInputs.addInput(CA);
     AllLinkerInputs.push_back(CA);
     break;
   }
@@ -2128,7 +2174,7 @@ void Driver::buildActions(SmallVectorImpl<const Action *> &TopLevelActions,
       !AllModuleInputs.empty()) {
     // We're performing multiple compilations; set up a merge module step
     // so we generate a single swiftmodule as output.
-    MergeModuleAction = C.createAction<MergeModuleJobAction>(AllModuleInputs);
+    MergeModuleAction = std::move(AllModuleInputs).intoAction(C);
   }
 
   bool shouldPerformLTO = OI.LTOVariant != OutputInfo::LTOKind::None;
@@ -2703,42 +2749,49 @@ static void addDiagFileOutputForPersistentPCHAction(
 
 /// If the file at \p input has not been modified since the last build (i.e. its
 /// mtime has not changed), adjust the Job's condition accordingly.
-static void
+static void handleCompileJobCondition(Job *J,
-handleCompileJobCondition(Job *J, CompileJobAction::InputInfo inputInfo,
+                                      CompileJobAction::InputInfo inputInfo,
-                          StringRef input, bool alwaysRebuildDependents) {
+                                      Optional<StringRef> input,
-  if (inputInfo.status == CompileJobAction::InputInfo::NewlyAdded) {
+                                      bool alwaysRebuildDependents) {
+  using InputStatus = CompileJobAction::InputInfo::Status;
+
+  if (inputInfo.status == InputStatus::NewlyAdded) {
     J->setCondition(Job::Condition::NewlyAdded);
     return;
   }
 
+  auto output = J->getOutput().getPrimaryOutputFilename();
   bool hasValidModTime = false;
   llvm::sys::fs::file_status inputStatus;
-  if (!llvm::sys::fs::status(input, inputStatus)) {
+  if (input.hasValue() && !llvm::sys::fs::status(*input, inputStatus)) {
+    J->setInputModTime(inputStatus.getLastModificationTime());
+    hasValidModTime = J->getInputModTime() == inputInfo.previousModTime;
+  } else if (!llvm::sys::fs::status(output, inputStatus)) {
     J->setInputModTime(inputStatus.getLastModificationTime());
     hasValidModTime = true;
   }
 
   Job::Condition condition;
-  if (hasValidModTime && J->getInputModTime() == inputInfo.previousModTime) {
+  if (hasValidModTime) {
     switch (inputInfo.status) {
-    case CompileJobAction::InputInfo::UpToDate:
+    case InputStatus::UpToDate:
-      if (llvm::sys::fs::exists(J->getOutput().getPrimaryOutputFilename()))
+      if (llvm::sys::fs::exists(output))
         condition = Job::Condition::CheckDependencies;
       else
         condition = Job::Condition::RunWithoutCascading;
       break;
-    case CompileJobAction::InputInfo::NeedsCascadingBuild:
+    case InputStatus::NeedsCascadingBuild:
       condition = Job::Condition::Always;
       break;
-    case CompileJobAction::InputInfo::NeedsNonCascadingBuild:
+    case InputStatus::NeedsNonCascadingBuild:
       condition = Job::Condition::RunWithoutCascading;
       break;
-    case CompileJobAction::InputInfo::NewlyAdded:
+    case InputStatus::NewlyAdded:
       llvm_unreachable("handled above");
     }
   } else {
     if (alwaysRebuildDependents ||
-        inputInfo.status == CompileJobAction::InputInfo::NeedsCascadingBuild) {
+        inputInfo.status == InputStatus::NeedsCascadingBuild) {
       condition = Job::Condition::Always;
     } else {
       condition = Job::Condition::RunWithoutCascading;
@@ -2895,14 +2948,18 @@ Job *Driver::buildJobsForAction(Compilation &C, const JobAction *JA,
   Job *J = C.addJob(std::move(ownedJob));
 
   // If we track dependencies for this job, we may be able to avoid running it.
-  if (!J->getOutput()
+  if (auto incrementalJob = dyn_cast<IncrementalJobAction>(JA)) {
-           .getAdditionalOutputForType(file_types::TY_SwiftDeps)
+    const bool alwaysRebuildDependents =
-           .empty()) {
+        C.getArgs().hasArg(options::OPT_driver_always_rebuild_dependents);
-    if (InputActions.size() == 1) {
+    if (!J->getOutput()
-      auto compileJob = cast<CompileJobAction>(JA);
+             .getAdditionalOutputForType(file_types::TY_SwiftDeps)
-      bool alwaysRebuildDependents =
+             .empty()) {
-          C.getArgs().hasArg(options::OPT_driver_always_rebuild_dependents);
+      if (InputActions.size() == 1) {
-      handleCompileJobCondition(J, compileJob->getInputInfo(), BaseInput,
+        handleCompileJobCondition(J, incrementalJob->getInputInfo(), BaseInput,
+                                  alwaysRebuildDependents);
+      }
+    } else if (isa<MergeModuleJobAction>(JA)) {
+      handleCompileJobCondition(J, incrementalJob->getInputInfo(), None,
                                 alwaysRebuildDependents);
     }
   }

test/Driver/Dependencies/one-way-merge-module-fine.swift

@@ -15,4 +15,4 @@
 
 // CHECK-SECOND-NOT: warning
 // CHECK-SECOND-NOT: Handled
-// CHECK-SECOND: Produced master.swiftmodule
+// CHECK-SECOND-NOT: Produced master.swiftmodule

test/Incremental/CrossModule/linear.swift

@@ -38,4 +38,21 @@
 // MODULE-B: Job finished: {merge-module: B.swiftmodule <= B.o}
 
 // MODULE-A: Job skipped: {compile: A.o <= A.swift}
-// MODULE-A: Job finished: {merge-module: A.swiftmodule <= A.o}
+// MODULE-A: Job skipped: {merge-module: A.swiftmodule <= A.o}
+
+//
+// And ensure that the null build really is null.
+//
+
+// RUN: cd %t && %swiftc_driver -c -incremental -emit-dependencies -emit-module -emit-module-path %t/C.swiftmodule -enable-experimental-cross-module-incremental-build -module-name C -I %t -output-file-map %t/C.json -working-directory %t -driver-show-incremental -driver-show-job-lifecycle -DNEW %t/C.swift 2>&1 | %FileCheck -check-prefix MODULE-C-NULL %s
+// RUN: cd %t && %swiftc_driver -c -incremental -emit-dependencies -emit-module -emit-module-path %t/B.swiftmodule -enable-experimental-cross-module-incremental-build -module-name B -I %t -output-file-map %t/B.json -working-directory %t -driver-show-incremental -driver-show-job-lifecycle %t/B.swift 2>&1 | %FileCheck -check-prefix MODULE-B-NULL %s
+// RUN: cd %t && %swiftc_driver -c -incremental -emit-dependencies -emit-module -emit-module-path %t/A.swiftmodule -enable-experimental-cross-module-incremental-build -module-name A -I %t -output-file-map %t/A.json -working-directory %t -driver-show-incremental -driver-show-job-lifecycle %t/A.swift 2>&1 | %FileCheck -check-prefix MODULE-A-NULL %s
+
+// MODULE-C-NULL: Job skipped: {compile: C.o <= C.swift}
+// MODULE-C-NULL: Job skipped: {merge-module: C.swiftmodule <= C.o}
+
+// MODULE-B-NULL: Job skipped: {compile: B.o <= B.swift}
+// MODULE-B-NULL: Job skipped: {merge-module: B.swiftmodule <= B.o}
+
+// MODULE-A-NULL: Job skipped: {compile: A.o <= A.swift}
+// MODULE-A-NULL: Job skipped: {merge-module: A.swiftmodule <= A.o}

test/Incremental/CrossModule/transitive.swift

@@ -42,3 +42,20 @@
 // MODULE-A: Queuing because of incremental external dependencies: {compile: A.o <= A.swift}
 // MODULE-A: Job finished: {compile: A.o <= A.swift}
 // MODULE-A: Job finished: {merge-module: A.swiftmodule <= A.o}
+
+//
+// And ensure that the null build really is null.
+//
+
+// RUN: cd %t && %swiftc_driver -c -incremental -emit-dependencies -emit-module -emit-module-path %t/C.swiftmodule -enable-experimental-cross-module-incremental-build -module-name C -I %t -output-file-map %t/C.json -working-directory %t -driver-show-incremental -driver-show-job-lifecycle -DUSEC -DNEW %t/C.swift 2>&1 | %FileCheck -check-prefix MODULE-C-NULL %s
+// RUN: cd %t && %swiftc_driver -c -incremental -emit-dependencies -emit-module -emit-module-path %t/B.swiftmodule -enable-experimental-cross-module-incremental-build -module-name B -I %t -output-file-map %t/B.json -working-directory %t -driver-show-incremental -driver-show-job-lifecycle -DUSEC %t/B.swift 2>&1 | %FileCheck -check-prefix MODULE-B-NULL %s
+// RUN: cd %t && %swiftc_driver -c -incremental -emit-dependencies -emit-module -emit-module-path %t/A.swiftmodule -enable-experimental-cross-module-incremental-build -module-name A -I %t -output-file-map %t/A.json -working-directory %t -driver-show-incremental -driver-show-job-lifecycle -DUSEC %t/A.swift 2>&1 | %FileCheck -check-prefix MODULE-A-NULL %s
+
+// MODULE-C-NULL: Job skipped: {compile: C.o <= C.swift}
+// MODULE-C-NULL: Job skipped: {merge-module: C.swiftmodule <= C.o}
+
+// MODULE-B-NULL: Job skipped: {compile: B.o <= B.swift}
+// MODULE-B-NULL: Job skipped: {merge-module: B.swiftmodule <= B.o}
+
+// MODULE-A-NULL: Job skipped: {compile: A.o <= A.swift}
+// MODULE-A-NULL: Job skipped: {merge-module: A.swiftmodule <= A.o}