Kill off some usages of the DeclChecker::IsFirstPass bit #15420

slavapestov · 2018-03-22T09:13:56Z

Follow-up to #15408. For most declaration kinds, moves all of the type checking work to the 'first pass'. Classes, enums, functions and pattern binding decls still remain. Once those are done, we'll be doing nothing in the 'second pass', at which point the flag and walk over the AST can be removed.

…by an Objective-C generic parameter In general it's fine to emit a diagnostic but leave the conformance valid here, we're not going to go on to emit code, and an ErrorType here causes other diagnostics to be not be emitted in a test due to minor validation order perturbations.

This is already handled by the common redeclaration check.

… pass'

…st pass'

…pass' Since protocol members no longer get visited in the 'second pass', change functions, constructors, associated types and pattern bindings to check accessibility in the first pass.

… pass'

The diagnostic change is harmless. The diagnostic that is no longer being emitted was intended for multi-file conformance checking only, and we still emit a diagnostic for the overall conformance failure with missing witnesses.

slavapestov · 2018-03-23T00:48:55Z

@swift-ci Please smoke test

slavapestov · 2018-03-23T00:49:02Z

@swift-ci Please test source compatibility

slavapestov · 2018-03-23T00:57:34Z

@swift-ci Please smoke test compiler performance

davezarzycki

Hi @slavapestov – I can mark this as reviewed, but I'm not sure what value I'm adding here. While I've been experimenting with the code recently, I don't feel like I intuitively understand it yet.

Also, if you weren't trying to rid us of IsFirstPass, I'd request that it be renamed to something semantically meaningful. :-)

What I'd really like to see (and this is separable feedback) is for some deduplication of logic in parallel Decl validation paths. For example, the nominal decls do very similar things for the most part, but it isn't clear which differences are essential and which are accidental.

slavapestov · 2018-03-23T02:24:15Z

@davezarzycki You've been working on this area recently cleaning up the various state bits and we had the forum discussion so I wanted to bring these PRs to your attention. I'm not looking for any specific feedback, but if you have any I'm happy to listen!

My goal is to completely eliminate IsFirstPass, I'm just doing it a bit at a time. I agree that if it made sense to keep a better name would help, but one of the reasons it doesn't make sense is that there isn't a good name for it, since it's not doing anything useful at this stage.

Cleaning up duplication between the various typeCheck and validateDecl cases would be nice. In addition to the ones you pointed out, there is also duplication between functions, constructors and destructors, in particular the manner in which we calculate the generic function type.

My approach so far as been to avoid attacking the 'surface-level' duplication, and instead try to simplify the underlying APIs so that there's less stuff to de-duplicate later. Also, I've been trying to make more fundamental structural changes to the declaration checker, like simplifying how we detect and track circularity, and this current project to eliminate passes. I hope that after enough iteration of this, the duplication will go away on its own.

swift-ci · 2018-03-23T02:37:19Z

Build comment file:

Summary for master smoketest

No regressions above thresholds

Debug
- debug brief
- debug detailed
Release
- release brief
- release detailed

Debug

debug brief

Regressed (0)

name	old	new	delta	delta_pct

Improved (0)

name	old	new	delta	delta_pct

Unchanged (delta < 1.0% or delta < 100.0ms) (2)

name	old	new	delta	delta_pct
LLVM.NumLLVMBytesOutput	47,005,894	47,004,940	-954	-0.0%
time.swift-driver.wall	80.0s	79.9s	-152.8ms	-0.19%

debug detailed

Regressed (0)

name	old	new	delta	delta_pct

Improved (0)

name	old	new	delta	delta_pct

Unchanged (delta < 1.0% or delta < 100.0ms) (23)

name	old	new	delta	delta_pct
AST.NumImportedExternalDefinitions	113,826	113,826	0	0.0%
AST.NumLoadedModules	16,158	16,158	0	0.0%
AST.NumTotalClangImportedEntities	333,024	333,024	0	0.0%
AST.NumUsedConformances	9,439	9,439	0	0.0%
IRModule.NumIRBasicBlocks	135,839	135,839	0	0.0%
IRModule.NumIRFunctions	80,197	80,197	0	0.0%
IRModule.NumIRGlobals	74,909	74,909	0	0.0%
IRModule.NumIRInsts	1,493,619	1,493,619	0	0.0%
IRModule.NumIRValueSymbols	138,281	138,281	0	0.0%
LLVM.NumLLVMBytesOutput	47,005,894	47,004,940	-954	-0.0%
SILModule.NumSILGenFunctions	51,279	51,279	0	0.0%
SILModule.NumSILOptFunctions	92,132	92,132	0	0.0%
Sema.NumConformancesDeserialized	469,915	469,963	48	0.01%
Sema.NumConstraintScopes	1,018,447	1,018,447	0	0.0%
Sema.NumDeclsDeserialized	3,050,340	3,050,356	16	0.0%
Sema.NumDeclsValidated	162,677	162,677	0	0.0%
Sema.NumFunctionsTypechecked	64,396	64,396	0	0.0%
Sema.NumGenericSignatureBuilders	85,473	85,496	23	0.03%
Sema.NumLazyGenericEnvironments	606,656	606,685	29	0.0%
Sema.NumLazyGenericEnvironmentsLoaded	50,009	50,032	23	0.05%
Sema.NumLazyIterableDeclContexts	439,475	439,475	0	0.0%
Sema.NumTypesDeserialized	3,288,959	3,288,100	-859	-0.03%
Sema.NumTypesValidated	272,638	272,637	-1	-0.0%

Release

release brief

Regressed (0)

name	old	new	delta	delta_pct

Improved (0)

name	old	new	delta	delta_pct

Unchanged (delta < 1.0% or delta < 100.0ms) (2)

name	old	new	delta	delta_pct
LLVM.NumLLVMBytesOutput	38,487,734	38,487,841	107	0.0%
time.swift-driver.wall	176.3s	176.7s	416.6ms	0.24%

release detailed

Regressed (0)

name	old	new	delta	delta_pct

Improved (0)

name	old	new	delta	delta_pct

Unchanged (delta < 1.0% or delta < 100.0ms) (23)

name	old	new	delta	delta_pct
AST.NumImportedExternalDefinitions	11,354	11,354	0	0.0%
AST.NumLoadedModules	462	462	0	0.0%
AST.NumTotalClangImportedEntities	31,515	31,515	0	0.0%
AST.NumUsedConformances	9,444	9,444	0	0.0%
IRModule.NumIRBasicBlocks	177,477	177,477	0	0.0%
IRModule.NumIRFunctions	61,660	61,660	0	0.0%
IRModule.NumIRGlobals	60,791	60,791	0	0.0%
IRModule.NumIRInsts	1,479,334	1,479,334	0	0.0%
IRModule.NumIRValueSymbols	110,755	110,755	0	0.0%
LLVM.NumLLVMBytesOutput	38,487,734	38,487,841	107	0.0%
SILModule.NumSILGenFunctions	26,022	26,022	0	0.0%
SILModule.NumSILOptFunctions	42,654	42,654	0	0.0%
Sema.NumConformancesDeserialized	157,586	157,586	0	0.0%
Sema.NumConstraintScopes	876,836	876,836	0	0.0%
Sema.NumDeclsDeserialized	258,196	258,196	0	0.0%
Sema.NumDeclsValidated	31,065	31,065	0	0.0%
Sema.NumFunctionsTypechecked	12,616	12,616	0	0.0%
Sema.NumGenericSignatureBuilders	7,667	7,675	8	0.1%
Sema.NumLazyGenericEnvironments	39,812	39,812	0	0.0%
Sema.NumLazyGenericEnvironmentsLoaded	5,035	5,035	0	0.0%
Sema.NumLazyIterableDeclContexts	24,627	24,627	0	0.0%
Sema.NumTypesDeserialized	316,553	316,254	-299	-0.09%
Sema.NumTypesValidated	44,130	44,130	0	0.0%

slavapestov · 2018-03-23T04:15:10Z

The SwifterSwift failure is in expression type checking and not related to my changes (and since the expression type checker ran it probably means the decl checker was OK).

slavapestov force-pushed the decl-checker-cleanup-part-2 branch from 2f28657 to e0d1ccc Compare March 22, 2018 09:47

slavapestov changed the title ~~[WIP] Kill off DeclChecker::IsFirstPass bit~~ Kill off some usages of the DeclChecker::IsFirstPass bit Mar 23, 2018

slavapestov added 9 commits March 22, 2018 17:47

Sema: Use -verify for Codable tests instead of FileCheck

a9b276c

Sema: Remove duplicate enum case check

025555f

This is already handled by the common redeclaration check.

Sema: DeclChecker::visitSubscriptDecl() does everything in the 'first…

3355a72

… pass'

Sema: DeclChecker::visitTypeAliasDecl() does everything in the 'first…

3131a16

… pass'

Sema: DeclChecker::visitEnumElementDecl() does everything in the 'fir…

63db382

…st pass'

Sema: DeclChecker::visitProtocolDecl() does everything in the 'first …

b7c1386

…pass' Since protocol members no longer get visited in the 'second pass', change functions, constructors, associated types and pattern bindings to check accessibility in the first pass.

Sema: DeclChecker::visitExtensionDecl() does everything in the 'first…

6382572

… pass'

slavapestov force-pushed the decl-checker-cleanup-part-2 branch from e0d1ccc to 24ac4f5 Compare March 23, 2018 00:48

slavapestov requested a review from davezarzycki March 23, 2018 00:54

davezarzycki approved these changes Mar 23, 2018

View reviewed changes

slavapestov merged commit a61cd12 into swiftlang:master Mar 23, 2018

slavapestov mentioned this pull request Apr 3, 2018

Remove more usages of DeclChecker::IsFirstPass #15453

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Kill off some usages of the DeclChecker::IsFirstPass bit #15420

Kill off some usages of the DeclChecker::IsFirstPass bit #15420

Uh oh!

slavapestov commented Mar 22, 2018 •

edited

Loading

Uh oh!

slavapestov commented Mar 23, 2018

Uh oh!

slavapestov commented Mar 23, 2018

Uh oh!

slavapestov commented Mar 23, 2018

Uh oh!

davezarzycki left a comment •

edited

Loading

Uh oh!

slavapestov commented Mar 23, 2018

Uh oh!

swift-ci commented Mar 23, 2018

Debug

debug brief

debug detailed

Release

release brief

release detailed

Uh oh!

slavapestov commented Mar 23, 2018

Uh oh!

Uh oh!

Kill off some usages of the DeclChecker::IsFirstPass bit #15420

Kill off some usages of the DeclChecker::IsFirstPass bit #15420

Uh oh!

Conversation

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

Uh oh!

Uh oh!

slavapestov commented Mar 23, 2018

Uh oh!

slavapestov commented Mar 23, 2018

Uh oh!

slavapestov commented Mar 23, 2018

Uh oh!

davezarzycki left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Mar 23, 2018

Uh oh!

swift-ci commented Mar 23, 2018

Build comment file:

Summary for master smoketest

Debug

debug brief

debug detailed

Release

release brief

release detailed

Uh oh!

slavapestov commented Mar 23, 2018

Uh oh!

Uh oh!

slavapestov commented Mar 22, 2018 •

edited

Loading

davezarzycki left a comment •

edited

Loading