Make `__consuming` meaningful for code generation. #19613

jckarter · 2018-09-28T20:41:52Z

Previously, the __consuming decl modifier failed to get propagated to the value ownership of the
method's self parameter, causing it to effectively be a no-op. Fix this, and address some of the
downstream issues this exposes:

coerceCallArguments in the type checker failing to handle the single __owned parameter case
Various places in SILGen and optimizer passes that made inappropriate assertions that self
was always passed guaranteed

rdar://problem/44769874

jckarter · 2018-09-28T20:42:06Z

@swift-ci Please test

jckarter · 2018-09-28T20:42:13Z

@swift-ci Please benchmark

jckarter · 2018-09-28T20:43:27Z

lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp

-        !AI->getSubstCalleeType()->getExtInfo().hasGuaranteedSelfParam())
+    if (Release && AI /*
+        && (!AI->getSubstCalleeType()->hasSelfParam()
+            || !AI->getSubstCalleeType()->getSelfParameter().isGuaranteed())*/)


@gottesmm Do you know what this hasGuaranteedSelfParam check was trying to accomplish? Removing doesn't appear to have any effect on the test suite, and this was the only remaining use of a broken SILFunctionType::hasGuaranteedSelfParam() method.

I would just rip it out.

jckarter · 2018-09-28T20:44:37Z

cc @airspeedswift

Previously, the `__consuming` decl modifier failed to get propagated to the value ownership of the method's `self` parameter, causing it to effectively be a no-op. Fix this, and address some of the downstream issues this exposes: - `coerceCallArguments` in the type checker failing to handle the single `__owned` parameter case - Various places in SILGen and optimizer passes that made inappropriate assertions that `self` was always passed guaranteed

jckarter · 2018-09-28T21:10:25Z

@swift-ci Please test

jckarter · 2018-09-28T21:10:36Z

@swift-ci Please benchmark

jckarter · 2018-09-28T21:10:43Z

@swift-ci Please benchmark

swift-ci · 2018-09-28T21:14:02Z

Build failed
Swift Test OS X Platform
Git Sha - bc49480dc293a54735894981898cf9d9cadbfaa7

swift-ci · 2018-09-28T21:29:18Z

Build failed
Swift Test Linux Platform
Git Sha - bc49480dc293a54735894981898cf9d9cadbfaa7

swift-ci · 2018-09-28T21:47:51Z

Build comment file:

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
IterateData	1565	1785	+14.1%	0.88x (?)
Improvement
LuhnAlgoLazy	454	295	-35.0%	1.54x
LuhnAlgoEager	438	294	-32.9%	1.49x
StringMatch	12235	11260	-8.0%	1.09x (?)
CharIteration_utf16_unicodeScalars	26670	24730	-7.3%	1.08x
RemoveWhereMoveInts	15	14	-6.7%	1.07x

Code size: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
IterateData.o	1388	1452	+4.6%	0.96x
Queue.o	12051	12451	+3.3%	0.97x
AnyHashableWithAClass.o	3604	3684	+2.2%	0.98x
RC4.o	5166	5230	+1.2%	0.99x
Improvement
LuhnAlgoEager.o	20131	18344	-8.9%	1.10x
LuhnAlgoLazy.o	20127	18341	-8.9%	1.10x
ArrayAppend.o	41843	39198	-6.3%	1.07x
StringWalk.o	48082	45242	-5.9%	1.06x
DictOfArraysToArrayOfDicts.o	35142	33894	-3.6%	1.04x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
COWArrayGuaranteedParameterOverhead	10134	11478	+13.3%	0.88x
DropLastAnyCollection	59	66	+11.9%	0.89x
IterateData	1640	1813	+10.5%	0.90x (?)
Improvement
SuffixAnyCollection	66	59	-10.6%	1.12x
StringMatch	12130	11141	-8.2%	1.09x (?)
FloatingPointPrinting_Float_description_uniform	5646	5271	-6.6%	1.07x

Code size: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
UTF8Decode.o	12983	13415	+3.3%	0.97x
IterateData.o	1564	1596	+2.0%	0.98x
RC4.o	4056	4120	+1.6%	0.98x
AnyHashableWithAClass.o	3884	3932	+1.2%	0.99x
LuhnAlgoLazy.o	15766	15942	+1.1%	0.99x
LuhnAlgoEager.o	15770	15946	+1.1%	0.99x
Improvement
StringWalk.o	40642	37290	-8.2%	1.09x
StringMatch.o	8097	7985	-1.4%	1.01x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Regression
CountAlgoString	4984	7263	+45.7%	0.69x
FatCompactMap	293511	352503	+20.1%	0.83x
WordCountUniqueUTF16	9973	11566	+16.0%	0.86x
ArrayOfPOD	758	860	+13.5%	0.88x (?)

Code size: Swift libraries

TEST	OLD	NEW	DELTA	RATIO
Improvement
libswiftStdlibUnittest.dylib	409600	405504	-1.0%	1.01x

How to read the data

The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false alarms. Unexpected regressions which are marked with '(?)' are probably noise. If you see regressions which you cannot explain you can try to run the benchmarks again. If regressions still show up, please consult with the performance team (@eeckstein).

Hardware Overview

  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

slavapestov · 2018-09-28T22:15:59Z

lib/Sema/CSApply.cpp

@@ -5753,6 +5753,17 @@ Expr *ExprRewriter::coerceCallArguments(
  // If we came from a scalar, create a scalar-to-tuple conversion.
  TupleShuffleExpr::TypeImpact typeImpact;
  if (argTuple == nullptr) {
+    // Deal with a difference in only scalar ownership.


Yuck. Hopefully this all goes away one day.

slavapestov · 2018-09-28T22:16:19Z

lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp

-        !AI->getSubstCalleeType()->getExtInfo().hasGuaranteedSelfParam())
+    if (Release && AI /*
+        && (!AI->getSubstCalleeType()->hasSelfParam()
+            || !AI->getSubstCalleeType()->getSelfParameter().isGuaranteed())*/)


I would just rip it out.

jckarter · 2018-09-28T23:39:44Z

@swift-ci Please test source compatibility

jckarter · 2018-09-29T01:36:13Z

Compat suite failure in 'Fluent' looks unrelated.

jckarter requested review from gottesmm, CodaFi and slavapestov September 28, 2018 20:41

jckarter commented Sep 28, 2018

View reviewed changes

jckarter force-pushed the enable-consuming branch from bc49480 to 9c5432c Compare September 28, 2018 21:10

slavapestov reviewed Sep 28, 2018

View reviewed changes

airspeedswift mentioned this pull request Sep 28, 2018

[stdlib] Mark reversed as consuming #19617

Merged

jckarter merged commit 5414747 into swiftlang:master Oct 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make `__consuming` meaningful for code generation. #19613

Make `__consuming` meaningful for code generation. #19613

Uh oh!

jckarter commented Sep 28, 2018 •

edited

Loading

Uh oh!

jckarter commented Sep 28, 2018

Uh oh!

jckarter commented Sep 28, 2018

Uh oh!

jckarter Sep 28, 2018

Uh oh!

slavapestov Sep 28, 2018

Uh oh!

jckarter commented Sep 28, 2018

Uh oh!

jckarter commented Sep 28, 2018

Uh oh!

jckarter commented Sep 28, 2018

Uh oh!

jckarter commented Sep 28, 2018

Uh oh!

swift-ci commented Sep 28, 2018

Uh oh!

swift-ci commented Sep 28, 2018

Uh oh!

swift-ci commented Sep 28, 2018

Uh oh!

slavapestov Sep 28, 2018

Uh oh!

slavapestov Sep 28, 2018

Uh oh!

jckarter commented Sep 28, 2018

Uh oh!

jckarter commented Sep 29, 2018

Uh oh!

Uh oh!

Make __consuming meaningful for code generation. #19613

Make __consuming meaningful for code generation. #19613

Uh oh!

Conversation

jckarter commented Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jckarter commented Sep 28, 2018

Uh oh!

jckarter commented Sep 28, 2018

Uh oh!

jckarter Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

slavapestov Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

jckarter commented Sep 28, 2018

Uh oh!

jckarter commented Sep 28, 2018

Uh oh!

jckarter commented Sep 28, 2018

Uh oh!

jckarter commented Sep 28, 2018

Uh oh!

swift-ci commented Sep 28, 2018

Uh oh!

swift-ci commented Sep 28, 2018

Uh oh!

swift-ci commented Sep 28, 2018

Build comment file:

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: Swift libraries

Uh oh!

slavapestov Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

slavapestov Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

jckarter commented Sep 28, 2018

Uh oh!

jckarter commented Sep 29, 2018

Uh oh!

Uh oh!

Make `__consuming` meaningful for code generation. #19613

Make `__consuming` meaningful for code generation. #19613

jckarter commented Sep 28, 2018 •

edited

Loading