Preserve SIL when merging modules #8388

slavapestov · 2017-03-28T09:31:10Z

Fixes rdar://problem/18913977.

Now that #11909 and previous PRs are in, we can finally look up associated types in imported conformances after Sema has been torn down. Phew!

Next steps in this area:

Emit default argument generators with shared linkage and serialized bit set in Swift 4 mode
Finish investigating emitting inlineable functions with shared linkage

slavapestov · 2017-03-28T09:32:27Z

So @jrose-apple, the problem is that while we constructed a SIL module with the right sections, nothing was forcing them to be read in. Also, I'm going to disable the SIL optimizer when there's no primary file. Does the general approach here look reasonable?

jrose-apple · 2017-03-28T16:09:22Z

You can't just disable the SIL optimizer when there's no primary file; that's WMO mode. Really in merge-modules we don't want to run any SIL passes—mandatory, optimizing, whatever. I took a manual stab at it at one point by passing -disable-sil-perf-optzns but today that still runs the "Onone passes". @swiftix, @gottesmm, would you be okay with -disable-sil-perf-optzns actually turning off all the optimization passes?

jrose-apple · 2017-03-28T16:09:47Z

lib/FrontendTool/FrontendTool.cpp

-    SM->verify();
+  if (!PrimarySourceFile) {
+    SM->linkAll();
+    SM->dump();


Clearly a WIP. :-)

I'm not sure what's being proposed, but -disable-sil-perf-optzns should be analogous to -disable-llvm-optzns. It should only disable optional SIL passes and should not affect functionality.

The Onone optimization passes are optional. (I'm not talking about the mandatory passes; this is the specific set of "optimizations run even at Onone".)

@atrick That is what I said below. They should add a different flag for this. @jrose-apple This brings up an interesting question. Right now we do not have that concept in our pipeline. Optimizations that run at -Onone are considered mandatory.

It's well understood that -Onone does not do what it says. The -disable flag should also disable the Onone optimizations. It's just that no one has bothered implementing it.

jrose-apple · 2017-03-28T16:11:09Z

include/swift/Serialization/SerializedSILLoader.h

@@ -112,6 +112,10 @@ class SerializedSILLoader {

  /// Deserialize all SILFunctions, VTables, and WitnessTables for
  /// a given Module.
+  void getAllForModule(Identifier Mod);


This is the same as passing nullptr for PrimaryFile below. I'm not sure if this is better because it's more explicit, or worse because it's easy to do by accident.

No it's the opposite. If you pass a null primary file below, we call getAll(true) on each iteration. I want to call getAll(false).

slavapestov · 2017-03-28T19:42:13Z

Ah. My bad on the primary file check. How do I detect that we have no source files?

jrose-apple · 2017-03-28T19:43:59Z

I don't know if that's the right check either. Last time I talked to @gottesmm about this we thought it probably made sense to add more SIL stages, and then be more careful about mixing them.

gottesmm

@jrose-apple I would rather add a separate flag than the perf flag. The reason why is that sometimes it is useful to just disable the perf passes despite doing at -O build.

gottesmm · 2017-03-28T20:31:05Z

include/swift/SIL/SILModule.h

@@ -459,6 +459,8 @@ class SILModule {
  /// Link in all VTables in the module.
  void linkAllVTables();

+  void linkAll();


I'm going to redo this and add comments etc. I just wanted to get feedback on the general approach(and it turns out I forgot WMO exists)

gottesmm · 2017-03-28T20:46:02Z

@slavapestov SGTM! In terms of general approach, IMO greater granularity is in general better. Not having to recompile when debugging is good.

slavapestov · 2017-04-01T05:00:36Z

@swift-ci Please test

slavapestov · 2017-04-01T05:00:48Z

@swift-ci Please test

slavapestov · 2017-04-01T05:01:57Z

This is now ready for review -- @graydon, @jrose-apple, @gottesmm.

swift-ci · 2017-04-01T05:45:17Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - f731ea506836fa927e80dbd9f34edd27c296b960
Test requested by - @slavapestov

slavapestov · 2017-04-01T06:03:26Z

Looks like swift-xcode-playground-support exposed a real bug here.

slavapestov · 2017-04-01T06:40:45Z

Reduced test case:

import Foundation

func replace(rgx: NSRegularExpression, in: String, with: String, x: NSRange) -> String {
  return rgx.stringByReplacingMatches(in: `in`, options: [], range: x, withTemplate: with)
}

It's crashing while deserializing the generic environment of _T0So19NSRegularExpressionC15MatchingOptionsVs10SetAlgebra10FoundationsAEPxqd__cs8SequenceRd__8Iterator_7ElementQYd__AJRtzlufCTW, which is a protocol conformance witness thunk:

1.	While deserializing decl #29 (XREF) in 'x'
2.	Cross-reference to module 'Foundation'
	... NSRegularExpression
	... in an extension in module 'Foundation'
	... MatchingOptions
	... Element

Of course the code compiles with -WMO or without -sil-merge-partial-modules so the driver or frontend must not be configuring something that would allow the imported decl to be found.

@jrose-apple Any ideas? Should be a simple oversight on my part.

jrose-apple · 2017-04-03T15:59:14Z

It could just be another case where SIL ends up pulling in new declarations from the Clang importer too late in the process. This isn't a new issue, but we mostly avoid it today by not having inlinable code that references Clang declarations. I'm not sure that simply declaring a TypeChecker that lives through the whole merge-modules phase is correct either—SILGen and SILOpt are probably set up to assume the set of external declarations doesn't change.

jrose-apple · 2017-04-03T15:59:36Z

That is, it's possible there's a whole army of yaks over this hill.

jrose-apple

I'm glad this is mostly coming out pretty simple, though!

jrose-apple · 2017-04-03T16:00:41Z

include/swift/AST/SILOptions.h

@@ -56,6 +56,11 @@ class SILOptions {
  /// Controls how to perform SIL linking.
  LinkingMode LinkMode = LinkNormal;

+  /// Controls whether to pull in SIL from partial modules during the
+  /// merge modules step. Could perhaps be merged with the link mode
+  /// above but the interactions between all the flags are tricky.


I think it's worth trying to merge them, in the spirit of "not leaving work around to clean up later". The settings are already not orthogonal.

jrose-apple · 2017-04-03T16:01:24Z

lib/Driver/ToolChains.cpp

+
+  // Disable SIL optimization passes; we've already optimized the code in each
+  // partial mode.
+  Arguments.push_back("-disable-diagnostic-passes");


*sigh* I wish this was inferred, but meanwhile this seems reasonable.

jrose-apple · 2017-04-03T16:01:39Z

lib/Driver/ToolChains.cpp

+  // Disable SIL optimization passes; we've already optimized the code in each
+  // partial mode.
+  Arguments.push_back("-disable-diagnostic-passes");
+  Arguments.push_back("-disable-sil-perf-optzns");


We should change this to also disable running the Onone passes.

@jrose-apple I think in such a case in our pipeline, we need a better separation conceptually in between the "diagnostic passes" and the "mandatory optimizations". Those should be conceptually two different groups of passes. I would rather have a separate flag. Perhaps:

-disable-sil-mandatory-optzns.

And then a third flag that just disables /all/ swift optzns, i.e.:

-disable-sil-optzns

I'm talking about SILPassPipelinePlan::getOnonePassPipeline. That's not mandatory optimizations.

Also, "mandatory optimizations" is an oxymoron, and therefore I wouldn't expect -disable-sil-optzns to disable the mandatory passes. Skipping the diagnostic passes and the mandatory passes should just be automatic from seeing the SIL stage.

I talked with @jrose-apple offline. Turns out my memory was wrong and the -Onone pass pipeline is not what I thought it was. The -Onone pass pipeline is a pass pipeline that includes extra optimizations run at -Onone.

slavapestov · 2017-04-03T21:19:09Z

For posterity here's a test case that triggers this problem even without my change.

x.swift:

import Foundation

@inline(__always)
public func replace(rgx: NSRegularExpression, in: String, with: String, x: NSRange) -> String {
  return rgx.stringByReplacingMatches(in: `in`, options: [], range: x, withTemplate: with)
}

y.swift:

import Foundation
import x

public func rreplace(rgx: NSRegularExpression, in: String, with: String, x: NSRange) -> String {
  return replace(rgx: rgx, in: `in`, with: with, x: x)
}

Compile x.swift with WMO, and y.swift with -O (doesn't matter if you enable WMO or not).

jrose-apple · 2017-04-03T22:11:54Z

Maybe we can throw that into the validation suite.

slavapestov · 2017-04-14T07:47:00Z

@DougGregor recently did some work on the ClangImporter. Let's try running the tests again.

slavapestov · 2017-04-14T07:47:04Z

@swift-ci Please test

swift-ci · 2017-04-14T08:10:41Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 72e40e9c3470efc993e14d002e9bddafc1fc0b46
Test requested by - @slavapestov

slavapestov · 2017-04-15T05:20:15Z

New failure, but in the same code:

01:11:46 0  swift                    0x0000000112164ee8 llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 40
01:11:46 1  swift                    0x0000000112165596 SignalHandler(int) + 454
01:11:46 2  libsystem_platform.dylib 0x00007fff9f740bba _sigtramp + 26
01:11:46 3  libsystem_platform.dylib 000000000000000000 _sigtramp + 1619784800
01:11:46 4  swift                    0x000000010fe3f848 swift::LookUpConformanceInSubstitutionMap::operator()(swift::CanType, swift::Type, swift::ProtocolType*) const + 40
01:11:46 5  swift                    0x000000010fe47b4e llvm::Optional<swift::ProtocolConformanceRef> llvm::function_ref<llvm::Optional<swift::ProtocolConformanceRef> (swift::CanType, swift::Type, swift::ProtocolType*)>::callback_fn<swift::LookUpConformanceInSubstitutionMap>(long, swift::CanType, swift::Type, swift::ProtocolType*) + 14
01:11:46 6  swift                    0x000000010fe3fe28 getMemberForBaseType(llvm::function_ref<llvm::Optional<swift::ProtocolConformanceRef> (swift::CanType, swift::Type, swift::ProtocolType*)>, swift::Type, swift::Type, swift::AssociatedTypeDecl*, swift::Identifier, swift::OptionSet<swift::SubstFlags, unsigned int>) + 648
01:11:46 7  swift                    0x000000010fe43e57 llvm::Optional<swift::Type> llvm::function_ref<llvm::Optional<swift::Type> (swift::TypeBase*)>::callback_fn<substType(swift::Type, llvm::function_ref<swift::Type (swift::SubstitutableType*)>, llvm::function_ref<llvm::Optional<swift::ProtocolConformanceRef> (swift::CanType, swift::Type, swift::ProtocolType*)>, swift::OptionSet<swift::SubstFlags, unsigned int>)::$_17>(long, swift::TypeBase*) + 1271
01:11:46 8  swift                    0x000000010fe40d83 swift::Type::transformRec(llvm::function_ref<llvm::Optional<swift::Type> (swift::TypeBase*)>) const + 163
01:11:46 9  swift                    0x000000010fe41003 swift::Type::transformRec(llvm::function_ref<llvm::Optional<swift::Type> (swift::TypeBase*)>) const + 803
01:11:46 10 swift                    0x000000010fe3f699 swift::Type::subst(llvm::function_ref<swift::Type (swift::SubstitutableType*)>, llvm::function_ref<llvm::Optional<swift::ProtocolConformanceRef> (swift::CanType, swift::Type, swift::ProtocolType*)>, swift::OptionSet<swift::SubstFlags, unsigned int>) const + 105
01:11:46 11 swift                    0x000000010f937c5a (anonymous namespace)::SILTypeSubstituter::visitType(swift::CanType) + 90
01:11:46 12 swift                    0x000000010f93774e swift::CanType swift::CanTypeVisitor<(anonymous namespace)::SILTypeSubstituter, swift::CanType>::visit<>(swift::CanType) + 94
01:11:46 13 swift                    0x000000010f93269f (anonymous namespace)::SILTypeSubstituter::substSILFunctionType(swift::CanTypeWrapper<swift::SILFunctionType>) + 623
01:11:46 14 swift                    0x000000010f9323e7 swift::SILFunctionType::substGenericArgs(swift::SILModule&, llvm::function_ref<swift::Type (swift::SubstitutableType*)>, llvm::function_ref<llvm::Optional<swift::ProtocolConformanceRef> (swift::CanType, swift::Type, swift::ProtocolType*)>) + 71
01:11:46 15 swift                    0x000000010f93231a swift::SILFunctionType::substGenericArgs(swift::SILModule&, swift::SubstitutionMap const&) + 58
01:11:46 16 swift                    0x000000010f93220a swift::SILFunctionType::substGenericArgs(swift::SILModule&, llvm::ArrayRef<swift::Substitution>) + 58
01:11:46 17 swift                    0x000000010f9c18ff (anonymous namespace)::SILVerifier::checkApplySubstitutions(llvm::ArrayRef<swift::Substitution>, swift::SILType) + 319
01:11:46 18 swift                    0x000000010f9c0a3c (anonymous namespace)::SILVerifier::checkFullApplySite(swift::FullApplySite) + 76
01:11:46 19 swift                    0x000000010f9a2277 swift::SILVisitor<(anonymous namespace)::SILVerifier, void>::visit(swift::ValueBase*) + 13735
01:11:46 20 swift                    0x000000010f99d883 (anonymous namespace)::SILVerifier::visitSILBasicBlock(swift::SILBasicBlock*) + 1315
01:11:46 21 swift                    0x000000010f997c97 swift::SILFunction::verify(bool) const + 8823
01:11:46 22 swift                    0x000000010f999d45 swift::SILModule::verify() const + 197
01:11:46 23 swift                    0x000000010ee35100 swift::performFrontend(llvm::ArrayRef<char const*>, char const*, void*, swift::FrontendObserver*) + 13360
01:11:46 24 swift                    0x000000010edef430 main + 3312
01:11:46 25 libdyld.dylib            0x00007fff9f533255 start + 1

slavapestov · 2017-04-15T05:20:57Z

@DougGregor looks like it got further, though. This is in SIL verification at the end of merge-modules.

DougGregor · 2017-04-17T15:50:08Z

You know, that backtrace looks a whole lot like something fixed by #8812.

DougGregor · 2017-04-17T15:50:11Z

@swift-ci Please test

swift-ci · 2017-04-17T16:24:33Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 72e40e9c3470efc993e14d002e9bddafc1fc0b46
Test requested by - @DougGregor

DougGregor · 2017-04-17T20:48:02Z

No such luck. Needs actually debugging

slavapestov · 2017-09-07T00:53:45Z

@swift-ci Please test macOS

slavapestov · 2017-09-14T21:05:20Z

@swift-ci Please test

slavapestov · 2017-09-14T21:05:29Z

@swift-ci Please test source compatibility

swift-ci · 2017-09-14T21:35:01Z

Build failed
Swift Test Linux Platform
Git Sha - a521f17

This test used to fail because we would look up an associated type in an incomplete conformance after Sema was torn down, causing a crash. Now, the ClangImporter is able to correctly complete the conformance.

Also pass flags to disable SIL optimization passes when merging modules, since that's completely unnecessary. An evolution test that used to fail with WMO disabled now passes with this change. FIxes <rdar://problem/18913977>.

DougGregor · 2017-09-14T22:54:17Z

Meh, you're missing a

REQUIRES: objc_interop

one of the new tests.

slavapestov · 2017-09-14T22:54:36Z

Source compat test suite passed. This new version just adds the REQUIRES:.

slavapestov · 2017-09-14T22:54:44Z

@swift-ci Please smoke test

slavapestov · 2017-09-14T22:54:48Z

@swift-ci Please smoke test

slavapestov · 2017-09-15T02:04:13Z

I think this is causing a failure on some of our internal bots -- reverting for now.

slavapestov changed the title ~~Preserve SIL when merging modules~~ [WIP] Preserve SIL when merging modules Mar 28, 2017

jrose-apple reviewed Mar 28, 2017

View reviewed changes

gottesmm reviewed Mar 28, 2017

View reviewed changes

slavapestov force-pushed the preserve-sil-when-merging-modules branch from 77de22f to f731ea5 Compare April 1, 2017 04:59

slavapestov changed the title ~~[WIP] Preserve SIL when merging modules~~ Preserve SIL when merging modules Apr 1, 2017

jrose-apple reviewed Apr 3, 2017

View reviewed changes

slavapestov mentioned this pull request Apr 4, 2017

Frontend: Add a new -sil-merge-partial-modules flag #8531

Merged

slavapestov force-pushed the preserve-sil-when-merging-modules branch from f731ea5 to 72e40e9 Compare April 14, 2017 07:46

slavapestov force-pushed the preserve-sil-when-merging-modules branch from 72e40e9 to 5c45ba8 Compare September 7, 2017 08:57

slavapestov changed the title ~~Preserve SIL when merging modules~~ [WIP] Preserve SIL when merging modules Sep 7, 2017

slavapestov force-pushed the preserve-sil-when-merging-modules branch from 5c45ba8 to a521f17 Compare September 14, 2017 21:05

slavapestov changed the title ~~[WIP] Preserve SIL when merging modules~~ Preserve SIL when merging modules Sep 14, 2017

slavapestov added 3 commits September 14, 2017 15:54

Add test case for imported conformance usage after Sema tear-down

07fe6ae

This test used to fail because we would look up an associated type in an incomplete conformance after Sema was torn down, causing a crash. Now, the ClangImporter is able to correctly complete the conformance.

Driver: Pass the new -sil-merge-partial-modules flag

ef58349

Also pass flags to disable SIL optimization passes when merging modules, since that's completely unnecessary. An evolution test that used to fail with WMO disabled now passes with this change. FIxes <rdar://problem/18913977>.

AST: Use getSuperclassForDecl() in one spot

cf9e266

slavapestov force-pushed the preserve-sil-when-merging-modules branch from a521f17 to cf9e266 Compare September 14, 2017 22:54

slavapestov merged commit ecf8f53 into swiftlang:master Sep 15, 2017

This was referenced Sep 15, 2017

Revert "Preserve SIL when merging modules" #11938

Merged

Re-apply "Preserve SIL when merging modules" #11943

Merged

Preserve SIL when merging modules #8388

Preserve SIL when merging modules #8388

Uh oh!

Conversation

slavapestov commented Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slavapestov commented Mar 28, 2017

Uh oh!

jrose-apple commented Mar 28, 2017

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!

slavapestov commented Mar 28, 2017

Uh oh!

jrose-apple commented Mar 28, 2017

Uh oh!

gottesmm 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!

gottesmm commented Mar 28, 2017

Uh oh!

slavapestov commented Apr 1, 2017

Uh oh!

slavapestov commented Apr 1, 2017

Uh oh!

slavapestov commented Apr 1, 2017

Uh oh!

swift-ci commented Apr 1, 2017

Uh oh!

slavapestov commented Apr 1, 2017

Uh oh!

slavapestov commented Apr 1, 2017

Uh oh!

jrose-apple commented Apr 3, 2017

Uh oh!

jrose-apple commented Apr 3, 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!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Apr 3, 2017

Uh oh!

jrose-apple commented Apr 3, 2017

Uh oh!

slavapestov commented Apr 14, 2017

Uh oh!

slavapestov commented Apr 14, 2017

Uh oh!

swift-ci commented Apr 14, 2017

slavapestov commented Mar 28, 2017 •

edited

Loading