[Frontend] Track SwiftOnoneSupport as a system dependency #18344

CodaFi · 2018-07-29T23:10:01Z

SwiftOnoneSupport is an implicit dependency of no-opt builds that is usually only loaded when frontend actions that emit optimization-sensitive outputs are run.

Force the implicit dependency to be explicit when -track-system-dependencies is used.

CodaFi · 2018-07-29T23:10:28Z

lib/Frontend/Frontend.cpp

-         RequestedAction == FrontendOptions::ActionType::EmitSIL;
+         RequestedAction == FrontendOptions::ActionType::EmitSIL ||
+         (TrackSystemDeps
+          && RequestedAction == FrontendOptions::ActionType::ResolveImports);


Is this even the right condition? Should we always force this load when TrackSystemDeps is true?

Another option would be to manually insert this dependency into the tracker if TrackSystemDeps is true but this condition in its original form returns false.

The problem for me is that the comment on the TrackSystemDeps option doesn't explain enough about the "why". Not your fault, is preexisting. But I cannot tell if this condition is correct from the comments.

jrose-apple · 2018-07-30T16:19:56Z

This definitely doesn't seem correct. If you only compile optimized builds, you don't depend on SwiftOnoneSupport. Are implicit imports just not being recorded at all?

CodaFi · 2018-07-30T16:43:15Z

This conditional occurs after the check for Onone builds. We’re currently specifically not loading SwiftOnoneSupport unless it returns true. The rest of the implicit dependencies are loaded and, therefore, tracked.

davidungar

Looks reasonable, but I have no idea if it's right or not.

davidungar · 2018-07-30T17:10:20Z

lib/Frontend/Frontend.cpp

-         RequestedAction == FrontendOptions::ActionType::EmitSIL;
+         RequestedAction == FrontendOptions::ActionType::EmitSIL ||
+         (TrackSystemDeps
+          && RequestedAction == FrontendOptions::ActionType::ResolveImports);


The problem for me is that the comment on the TrackSystemDeps option doesn't explain enough about the "why". Not your fault, is preexisting. But I cannot tell if this condition is correct from the comments.

jrose-apple · 2018-07-30T17:32:11Z

Ah, I get it now. This is a minor optimization to speed up compile time if we're not going to get to SIL optimization. I think you should just make the condition be TrackSystemDeps, then, and not check the action type; as written it'll load it for -resolve-imports but not -typecheck, which doesn't really make sense. And David's right that this needs explaining.

CodaFi · 2018-07-30T17:42:58Z

I've updated the patch to always load SwiftOnoneSupport in the presence of -track-system-dependencies.

jrose-apple · 2018-07-30T17:58:40Z

lib/Frontend/Frontend.cpp

@@ -421,7 +427,8 @@ shouldImplicityImportSwiftOnoneSupportModule(CompilerInvocation &Invocation) {
    return false;

  if (shouldImportSwiftOnoneModuleIfNoneOrImplicitOptimization(
-          Invocation.getFrontendOptions().RequestedAction)) {
+          Invocation.getFrontendOptions().RequestedAction,
+          Invocation.getFrontendOptions().TrackSystemDeps)) {
    return true;
  }
  return Invocation.getFrontendOptions().isCreatingSIL();


This bit here's still going to trip you up, yeah? Actually, isn't this basically the same as what shouldImportSwiftOnoneModuleIfNoneOrImplicitOptimization is doing?

Oh, right. Meant to collapse these!

jrose-apple · 2018-07-30T18:12:38Z

lib/Frontend/Frontend.cpp

+  // This optimization is disabled by -track-system-dependencies to preserve
+  // the explicit dependency.
+  return Invocation.getFrontendOptions().TrackSystemDeps
+      || Invocation.getFrontendOptions().isCreatingSIL();


Strictly speaking you've turned on imports for SILGen too. Then again, the old logic would have missed EmitSIB and all the various IR-based options, so this is probably more future-proof. Still, it might slow down the tests; it may be worth special-casing SILGen to not do the import.

Hm, this is the only use of this accessor. I'll make it stricter.

CodaFi · 2018-07-30T20:54:11Z

@swift-ci please smoke test

jrose-apple

Looks good, only small comments.

jrose-apple · 2018-07-30T21:24:23Z

include/swift/Frontend/FrontendOptions.h

@@ -310,6 +308,7 @@ class FrontendOptions {
  static bool canActionEmitInterface(ActionType);

 public:
+  static bool doesActionNeedSILOptimizer(ActionType);


Bikeshedding: I don't like this name because it's not the optimizer itself that's interesting. doesActionRunSILPasses, maybe?

jrose-apple · 2018-07-30T21:24:59Z

lib/Frontend/FrontendOptions.cpp

+  case ActionType::EmitSIL:
+  case ActionType::EmitModuleOnly:
+  case ActionType::MergeModules:
+  case ActionType::EmitSIBGen:


If we're going to change behavior anyway, we may as well pull EmitSIBGen out of this list.

SwiftOnoneSupport is an implicit dependency of no-opt builds that is usually only loaded when frontend actions that emit optimization-sensitive outputs are run. Force the implicit dependency to be explicit when -track-system-dependencies is used in concert with frontend actions that requires SIL passes be run.

CodaFi · 2018-07-30T23:40:17Z

@swift-ci please smoke test

CodaFi · 2018-07-31T01:16:35Z

⛵️

Continuing work from swiftlang#18344, be more conservative about when we load SwiftOnoneSupport. Specifically, -emit-silgen and -emit-sibgen, despite not going through the SIL Optimizer, may silently introduce dependencies on SwiftOnoneSupport. Because we want to support the ability to posthumously compile SILGen and SIBGen'd files with these implicit dependencies, and because SIL is not yet capable of expressing the dependency itself, we must always assume we need to load SwiftOnoneSupport.

CodaFi requested review from davidungar and jrose-apple July 29, 2018 23:10

CodaFi commented Jul 29, 2018

View reviewed changes

davidungar reviewed Jul 30, 2018

View reviewed changes

CodaFi force-pushed the unseen-university branch 2 times, most recently from ba1a044 to ef17087 Compare July 30, 2018 17:41

jrose-apple reviewed Jul 30, 2018

View reviewed changes

CodaFi force-pushed the unseen-university branch from ef17087 to d1c59f3 Compare July 30, 2018 18:08

jrose-apple reviewed Jul 30, 2018

View reviewed changes

CodaFi force-pushed the unseen-university branch 3 times, most recently from ff14484 to 8c1634c Compare July 30, 2018 20:53

jrose-apple reviewed Jul 30, 2018

View reviewed changes

CodaFi force-pushed the unseen-university branch from 8c1634c to cdbb225 Compare July 30, 2018 21:54

CodaFi force-pushed the unseen-university branch from cdbb225 to 0a3731d Compare July 30, 2018 23:40

CodaFi merged commit f74d61f into swiftlang:master Jul 31, 2018

CodaFi deleted the unseen-university branch July 31, 2018 01:16

CodaFi mentioned this pull request Jul 31, 2018

Refine The Frontend's Understanding of SwiftOnoneSupport #18410

Merged

[Frontend] Track SwiftOnoneSupport as a system dependency #18344

[Frontend] Track SwiftOnoneSupport as a system dependency #18344

Uh oh!

Conversation

CodaFi commented Jul 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

jrose-apple commented Jul 30, 2018

Uh oh!

CodaFi commented Jul 30, 2018

Uh oh!

davidungar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Jul 30, 2018

Uh oh!

CodaFi commented Jul 30, 2018

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!

CodaFi commented Jul 30, 2018

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!

CodaFi commented Jul 30, 2018

Uh oh!

CodaFi commented Jul 31, 2018

Uh oh!

Uh oh!

CodaFi commented Jul 29, 2018 •

edited

Loading