Skip to content

[Sema] NFC - Bypass more Pattern type variables #15545

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 28, 2018
Merged

[Sema] NFC - Bypass more Pattern type variables #15545

merged 1 commit into from
Mar 28, 2018

Conversation

davezarzycki
Copy link
Contributor

Rather than throw away the result of simplifying the expression if the resulting is InOutType, see if the user explicitly created the InOutType. If they did not, then the RValue type is fine.

Hi @rudkx – Based on our conversation in #15542, this change makes InOut on the right hand side work. Right?

@davezarzycki
[Sema] NFC - Bypass more Pattern type variables
b97ec69 
Rather than throw away the result of simplifying the expression if the
resulting is InOutType, see if the user explicitly created the
InOutType. If they did not, then the RValue type is fine.
@davezarzycki davezarzycki requested a review from rudkx March 27, 2018 18:46
@davezarzycki
Copy link
Contributor Author

@swift-ci please smoke test

@CodaFi
Copy link
Contributor

CodaFi commented Mar 27, 2018

Thank you for continuing the war on InOutType ❤️

@davezarzycki
Copy link
Contributor Author

Hi @CodaFi – I actually don't find InOutType to be bothersome, but I get why other people do. :-)

If it were up to me, I'd actually merge InOutType, LValueType, the meta-type logic, DynamicSelfType, etc, etc into a QualifiedType type.

@CodaFi
Copy link
Contributor

CodaFi commented Mar 27, 2018

Calling conventions are not types - types are not calling conventions. It’s the right abstraction at the wrong level and misapplied. The semantic AST and pre-semantic AST should not care about it.

@davezarzycki
Copy link
Contributor Author

As I said, I get why people are bothered by InOutType. ;-)

@davezarzycki davezarzycki requested a review from CodaFi March 28, 2018 00:33
CodaFi
CodaFi reviewed Mar 28, 2018
View reviewed changes
lib/Sema/CSGen.cpp
return boundExprTy->getRValueType();
if (auto boundExpr = locator.trySimplifyToExpr()) {
if (!boundExpr->isSemanticallyInOutExpr())
return CS.getType(boundExpr)->getRValueType();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rudkx Was there a specific reason this wasn't using the type map before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean CS.getType()? That's just a couple more lines up in the diff.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, carry on then.

CodaFi
CodaFi approved these changes Mar 28, 2018
View reviewed changes
Copy link
Contributor

@CodaFi CodaFi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo a question.

@davezarzycki
Copy link
Contributor Author

@swift-ci please smoke test compiler performance

@swift-ci
Copy link
Contributor

Build comment file:

Summary for master smoketest

No regressions above thresholds

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,060,510 47,061,032 522 0.0%
time.swift-driver.wall 78.7s 78.9s 217.3ms 0.28%

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 337,992 337,992 0 0.0%
AST.NumUsedConformances 9,443 9,443 0 0.0%
IRModule.NumIRBasicBlocks 135,538 135,538 0 0.0%
IRModule.NumIRFunctions 80,153 80,153 0 0.0%
IRModule.NumIRGlobals 74,298 74,298 0 0.0%
IRModule.NumIRInsts 1,498,796 1,498,796 0 0.0%
IRModule.NumIRValueSymbols 137,719 137,719 0 0.0%
LLVM.NumLLVMBytesOutput 47,060,510 47,061,032 522 0.0%
SILModule.NumSILGenFunctions 49,967 49,967 0 0.0%
SILModule.NumSILOptFunctions 90,409 90,409 0 0.0%
Sema.NumConformancesDeserialized 472,076 472,076 0 0.0%
Sema.NumConstraintScopes 942,707 942,707 0 0.0%
Sema.NumDeclsDeserialized 3,041,418 3,041,418 0 0.0%
Sema.NumDeclsValidated 162,677 162,677 0 0.0%
Sema.NumFunctionsTypechecked 64,396 64,396 0 0.0%
Sema.NumGenericSignatureBuilders 85,006 85,006 0 0.0%
Sema.NumLazyGenericEnvironments 574,090 574,090 0 0.0%
Sema.NumLazyGenericEnvironmentsLoaded 50,027 50,027 0 0.0%
Sema.NumLazyIterableDeclContexts 437,052 437,052 0 0.0%
Sema.NumTypesDeserialized 3,265,162 3,265,162 0 0.0%
Sema.NumTypesValidated 271,465 271,465 0 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,518,921 38,518,929 8 0.0%
time.swift-driver.wall 170.8s 170.7s -96.5ms -0.06%

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 32,031 32,031 0 0.0%
AST.NumUsedConformances 9,448 9,448 0 0.0%
IRModule.NumIRBasicBlocks 177,873 177,873 0 0.0%
IRModule.NumIRFunctions 61,896 61,896 0 0.0%
IRModule.NumIRGlobals 60,195 60,195 0 0.0%
IRModule.NumIRInsts 1,481,807 1,481,807 0 0.0%
IRModule.NumIRValueSymbols 110,456 110,456 0 0.0%
LLVM.NumLLVMBytesOutput 38,518,921 38,518,929 8 0.0%
SILModule.NumSILGenFunctions 25,070 25,070 0 0.0%
SILModule.NumSILOptFunctions 42,180 42,180 0 0.0%
Sema.NumConformancesDeserialized 152,043 152,043 0 0.0%
Sema.NumConstraintScopes 820,246 820,246 0 0.0%
Sema.NumDeclsDeserialized 255,520 255,520 0 0.0%
Sema.NumDeclsValidated 31,065 31,065 0 0.0%
Sema.NumFunctionsTypechecked 12,616 12,616 0 0.0%
Sema.NumGenericSignatureBuilders 7,610 7,610 0 0.0%
Sema.NumLazyGenericEnvironments 37,647 37,647 0 0.0%
Sema.NumLazyGenericEnvironmentsLoaded 5,033 5,033 0 0.0%
Sema.NumLazyIterableDeclContexts 24,467 24,467 0 0.0%
Sema.NumTypesDeserialized 312,243 312,243 0 0.0%
Sema.NumTypesValidated 42,958 42,958 0 0.0%

@davezarzycki davezarzycki merged commit 99ccfb1 into swiftlang:master Mar 28, 2018
@davezarzycki davezarzycki deleted the nfc_check_InOutExpr_not_InOutType branch March 28, 2018 11:23
@davezarzycki davezarzycki mentioned this pull request Mar 28, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CodaFi CodaFi CodaFi approved these changes

@rudkx rudkx Awaiting requested review from rudkx

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@davezarzycki @CodaFi @swift-ci