Skip to content

Conversation

ravikandhadai
Copy link
Contributor

@ravikandhadai ravikandhadai commented Feb 4, 2020

This semantics attribute is used by the top-level array initializer (in ArrayShared.swift),
which is the entry point used by the compiler to initialize array from array literals.
This initializer is "early inlined" so that other optimizations can work on its body.

This PR also updates DeadObjectElimination and ArrayCOWOpts optimization passes to work with this semantics attribute in addition to "array.uninitialized", which they already use. (But this will not make them more effective as they are also applied after early inlining.)

It also refactors mapInitializationStores function from ArrayElementValuePropagation.cpp to
ArraySemantic.cpp so that the array-initialization pattern matching functionality
implemented by the function can be reused by other optimizations.

The purpose of this PR is to reuse the functionality implemented in ArraySemantics such as getArrayValue, getStoragePointer etc. in a new "mandatory" pass that will unroll forEach loops over array literals, which itself is aimed at optimizing the new os log calls (see #29651).

@ravikandhadai
Copy link
Contributor Author

@swift-ci Please test

@ravikandhadai
Copy link
Contributor Author

@swift-ci Please benchmark

@swift-ci
Copy link
Contributor

swift-ci commented Feb 4, 2020

Performance: -O

Regression OLD NEW DELTA RATIO
ClassArrayGetter2 50 60 +20.0% 0.83x (?)

Code size: -O

Performance: -Osize

Regression OLD NEW DELTA RATIO
SuffixCountableRangeLazy 4 6 +50.0% 0.67x (?)
PrefixWhileCountableRange 15 17 +13.3% 0.88x (?)

Code size: -Osize

Performance: -Onone

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: 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

@ravikandhadai
Copy link
Contributor Author

@swift-ci Please benchmark

@swift-ci
Copy link
Contributor

swift-ci commented Feb 4, 2020

Performance: -O

Regression OLD NEW DELTA RATIO
ObjectiveCBridgeStubFromArrayOfNSString2 2920 3140 +7.5% 0.93x (?)

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Improvement OLD NEW DELTA RATIO
DictionaryBridgeToObjC_Access 1057 924 -12.6% 1.14x (?)

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 Pro
  Model Identifier: MacPro6,1
  Processor Name: 8-Core Intel Xeon E5
  Processor Speed: 3 GHz
  Number of Processors: 1
  Total Number of Cores: 8
  L2 Cache (per Core): 256 KB
  L3 Cache: 25 MB
  Memory: 64 GB

Copy link
Contributor

@eeckstein eeckstein left a comment

Choose a reason for hiding this comment

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

lgtm!
I mentioned some cosmetic issues in my comments.

@@ -652,7 +671,20 @@ SILValue swift::ArraySemanticsCall::getArrayValue() const {
}

SILValue swift::ArraySemanticsCall::getArrayElementStoragePointer() const {
if (getKind() == ArrayCallKind::kArrayUninitialized) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function looks almost exactly the same as getArrayValue(), except the tuple element number (and the special handling of ArrayCallKind::kArrayInit in getArrayValue).
Can you extract the common code into a helper function? e.g. getArrayInitResult(int tupleElement)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. That is a good idea.

} else if (SI)
return false;

// Store an index_addr projection.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: Store to an ...

…zed_intrinsic"

semantics attribute that is used by the top-level array initializer (in ArrayShared.swift),
which is the entry point used by the compiler to initialize array from array literals.
This initializer is early-inlined so that other optimizations can work on its body.

Fix DeadObjectElimination and ArrayCOWOpts optimization passes to work with this
semantics attribute in addition to "array.uninitialized", which they already use.

Refactor mapInitializationStores function from ArrayElementValuePropagation.cpp to
ArraySemantic.cpp so that the array-initialization pattern matching functionality
implemented by the function can be reused by other optimizations.
@ravikandhadai
Copy link
Contributor Author

@swift-ci Please test and merge

1 similar comment
@ravikandhadai
Copy link
Contributor Author

@swift-ci Please test and merge

@ravikandhadai
Copy link
Contributor Author

@swift-ci Please test macOS Platform

1 similar comment
@ravikandhadai
Copy link
Contributor Author

@swift-ci Please test macOS Platform

@swift-ci
Copy link
Contributor

swift-ci commented Feb 5, 2020

Build failed
Swift Test OS X Platform
Git Sha - 72243bfd7ff4b511e8966ada74eccfc94b176eac

@ravikandhadai ravikandhadai merged commit bca5511 into swiftlang:master Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants