StringOptimization: handle static let variables for String constant folding. #33232

eeckstein · 2020-07-31T16:45:40Z

For example, constant fold:

struct Str {
    static let s = "hello"
}
...
let x = "<\(Str.s)>"

This PR also contains some other minor changes which are required for this optimization. For details see the commit list.

eeckstein · 2020-07-31T16:45:54Z

@swift-ci test

eeckstein · 2020-07-31T16:46:59Z

@swift-ci benchmark

swift-ci · 2020-07-31T17:18:23Z

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
LessSubstringSubstring	22	30	+36.4%	0.73x
LessSubstringSubstringGenericComparable	22	30	+36.4%	0.73x
EqualSubstringSubstring	22	29	+31.8%	0.76x (?)
EqualSubstringSubstringGenericEquatable	22	29	+31.8%	0.76x
EqualSubstringString	22	29	+31.8%	0.76x (?)
EqualStringSubstring	23	30	+30.4%	0.77x
PrefixWhileAnySequence	1067	1194	+11.9%	0.89x (?)
StringBuilderWithLongSubstring	1160	1280	+10.3%	0.91x (?)
StringComparison_longSharedPrefix	343	372	+8.5%	0.92x (?)

Improvement	OLD	NEW	DELTA	RATIO
StringFromLongWholeSubstring	8	2	-75.0%	4.00x
StringFromLongWholeSubstringGeneric	9	3	-66.7%	3.00x
FlattenListFlatMap	4458	3413	-23.4%	1.31x (?)
ObjectiveCBridgeStringHash	90	80	-11.1%	1.12x
IterateData	964	864	-10.4%	1.12x (?)

Code size: -O

Performance: -Osize

Regression	OLD	NEW	DELTA	RATIO
EqualSubstringString	22	30	+36.4%	0.73x
EqualStringSubstring	22	29	+31.8%	0.76x
LessSubstringSubstring	23	30	+30.4%	0.77x
EqualSubstringSubstringGenericEquatable	23	30	+30.4%	0.77x
LessSubstringSubstringGenericComparable	23	30	+30.4%	0.77x
EqualSubstringSubstring	23	29	+26.1%	0.79x
StringComparison_longSharedPrefix	342	372	+8.8%	0.92x (?)

Improvement	OLD	NEW	DELTA	RATIO
StringFromLongWholeSubstring	8	2	-75.0%	4.00x
StringFromLongWholeSubstringGeneric	9	3	-66.7%	3.00x
ObjectiveCBridgeStringHash	90	79	-12.2%	1.14x (?)
NopDeinit	11000	9900	-10.0%	1.11x
StringHashing_latin1	226	208	-8.0%	1.09x (?)
StringHashing_fastPrenormal	650	600	-7.7%	1.08x (?)
StringWalk	1640	1520	-7.3%	1.08x (?)
ArrayAppendUTF16Substring	31752	29664	-6.6%	1.07x (?)

Code size: -Osize

Performance: -Onone

Regression	OLD	NEW	DELTA	RATIO
EqualSubstringSubstringGenericEquatable	26	33	+26.9%	0.79x
LessSubstringSubstringGenericComparable	27	34	+25.9%	0.79x (?)
LessSubstringSubstring	29	35	+20.7%	0.83x (?)
EqualStringSubstring	29	35	+20.7%	0.83x
EqualSubstringString	29	35	+20.7%	0.83x (?)
EqualSubstringSubstring	29	34	+17.2%	0.85x (?)

Improvement	OLD	NEW	DELTA	RATIO
ObjectiveCBridgeStringHash	91	80	-12.1%	1.14x
StringWalk	3360	3040	-9.5%	1.11x (?)
StrComplexWalk	5470	4980	-9.0%	1.10x (?)

Code size: -swiftlibs

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 mini
  Model Identifier: Macmini8,1
  Processor Name: 6-Core Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 64 GB

swift-ci · 2020-07-31T18:07:15Z

Build failed
Swift Test OS X Platform
Git Sha - e1371739a9bb72a33bf6ca5cf80c91170b3c7ccc

atrick

I'm not really familiar with StringOptimizer, but this basically looks fine.

These pass pipeline changes conflict with the changes I've been trying to make for a long time
#32659

Essentially, we should not destroy semantics in any way during the mid-level pipeline stage. So semantics calls, global initializers, and semantics types should only be inlined or expanded by the low-level pipeline stage as part of lowering. The "high level" pipeline stage should only be for things that need to run before serialization. I'm not sure if StringOptimizer should run before serialization.

atrick · 2020-08-01T03:36:50Z

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

         "Unexpected promotion of address-only type!");

-  assert(NewValue && "Expected a value to release!");
+  assert(NewValue || (Ty.is<TupleType>() && Ty.getAs<TupleType>()->getNumElements() == 0));


It looks like this assert is supposed to protect the call to emitLoweredDestroyValue. I don't think a new destroy should be created for an invalid SILValue.

This assert makes sure that a value is actually written to the location before it is destroyed.
But this is not required for empty tuples.

@eeckstein Disabling this assert means that an invalid SILValue is passed to emitLoweredDestroyValue. That is incorrect.

emitLoweredDestroyValue is a no-op for an empty tuple.

I get that it might happen to work, or might not. I'm just being picky about something trivial to make a point about interfaces. emitLoweredDestroyValue does not specify that it can take an invalid SILValue, and there's no single header-level implementation to check, so to anyone trying to understand this code it is incorrect at face value. It doesn't matter whether emitLoweredDestroyValue is actually going to emit an instruction or not. In general, if an interface can accept an invalid SILValue that needs to be specified in the interface. The same is true for accepting null pointers.

i.e. clearly written code would look like

if (!newValue) { assert(Ty.is<Tuple...) return }

ok, that's a good point

atrick · 2020-08-01T03:45:08Z

lib/SILOptimizer/PassManager/PassPipeline.cpp

@@ -290,7 +290,11 @@ void addFunctionPasses(SILPassPipelinePlan &P,
  P.addLowerAggregateInstrs();

  // Split up operations on stack-allocated aggregates (struct, tuple).
-  P.addSROA();
+  if (OpLevel == OptimizationLevelKind::HighLevel) {


I think semantics should be preserved throughout the mid-level pipeline, and the low-level SIL pipeline should just be a series of lowering passes... although this is fine for now

atrick · 2020-08-01T03:48:30Z

lib/SILOptimizer/PassManager/PassPipeline.cpp

@@ -493,6 +493,8 @@ static void addHighLevelFunctionPipeline(SILPassPipelinePlan &P) {
  addFunctionPasses(P, OptimizationLevelKind::HighLevel);

  addHighLevelLoopOptPasses(P);
+
+  P.addStringOptimization();


Does string optimization need to be done before serialization for some reason? If not, I think it should stay in the mid-level pipeline an SROA can avoid splitting them up into low-level "lowering". Does splitting Strings up enable other mid-level optimization?

Global initializers should not be optimized in mid-level SIL at all

I fixed this here, which is now in @meg-gupta 's PR, but it still hasn't been merged
7c9e8b8

It doesn't need to be before Serialization, it just has to be before the mid-level pipeline: the reason is that, even if StringOptimization is at the begin of the mid-level function pipeline, it can be that the whole mid-level function pipeline already has run for the global initializer (which means that the string init is inlined). And then the optimization cannot analyze the initializer anymore.

As we don't have a function-pipeline between serialization and the midlevel function pipeline, I put it into the high-level function pipeline.

@meg-gupta Feel free to change that in 7c9e8b8. The pipeline just needs to make sure that all string operations are not inlined (including in the initializers) when StringOptimization runs.

atrick · 2020-08-01T04:03:15Z

lib/SILOptimizer/Transforms/StringOptimization.cpp

+StringOptimization::StringInfo
+StringOptimization::getStringFromStaticLet(SILValue value) {
+  // Match the pattern
+  //   %ptr_to_global = apple %addressor()


s/apple/apply

NFC

An empty tuple doesn't need to be assigned before it is passed to destroy_addr. I found this by experimenting with the pass pipeline. Not sure if this crash can happen with the current pipeline.

…sn't split String types. The StringOptimization relies on seeing String values a a whole and not being split.

Needed to make sure that global initializers are not optimized in mid-level SIL while other functions are still in high-level SIL. Having the StringOptimization not in high-level SIL was just a mistake in my earlier PR.

…olding. For example, constant fold: struct Str { static let s = "hello" } ... let x = "<\(Str.s)>"

eeckstein · 2020-08-03T10:01:58Z

@swift-ci smoke test

eeckstein · 2020-08-03T10:02:14Z

@swift-ci smoke test

eeckstein requested a review from atrick July 31, 2020 16:47

atrick mentioned this pull request Aug 1, 2020

[WIP] Change Inliner heuristics w.r.t semantic functions #32659

Closed

atrick approved these changes Aug 1, 2020

View reviewed changes

eeckstein added 5 commits August 3, 2020 12:01

SIL: small refactoring of swift::findInitializer

ba7ecfb

NFC

Mem2Reg: fix a wrong assert

568c156

An empty tuple doesn't need to be assigned before it is passed to destroy_addr. I found this by experimenting with the pass pipeline. Not sure if this crash can happen with the current pipeline.

SILOptimizer: make a separate SROA pass for high-level SIL, which doe…

2a03543

…sn't split String types. The StringOptimization relies on seeing String values a a whole and not being split.

StringOptimization: handle static let variables for String constant f…

e3f3b75

…olding. For example, constant fold: struct Str { static let s = "hello" } ... let x = "<\(Str.s)>"

eeckstein force-pushed the string-optimization branch from e137173 to e3f3b75 Compare August 3, 2020 10:01

eeckstein merged commit 9bac363 into swiftlang:master Aug 3, 2020

eeckstein deleted the string-optimization branch August 3, 2020 12:57

StringOptimization: handle static let variables for String constant folding. #33232

StringOptimization: handle static let variables for String constant folding. #33232

Uh oh!

Conversation

eeckstein commented Jul 31, 2020

Uh oh!

eeckstein commented Jul 31, 2020

Uh oh!

eeckstein commented Jul 31, 2020

Uh oh!

swift-ci commented Jul 31, 2020

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

swift-ci commented Jul 31, 2020

Uh oh!

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

atrick Aug 5, 2020 • 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!

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!

eeckstein Aug 3, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

eeckstein commented Aug 3, 2020

Uh oh!

eeckstein commented Aug 3, 2020

Uh oh!

Uh oh!

atrick Aug 5, 2020 •

edited

Loading

eeckstein Aug 3, 2020 •

edited

Loading