Skip to content

[WIP] [NO-MERGE] Make "unique" flag more robust and update access enforcement opts to make use of it. #32877

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.

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

Closed

Conversation

zoecarver
Copy link
Contributor

This is a WIP PR to get a benchmark of the performance improvements of access enforcement opts when it promotes uniquely identified storage accesses to statically enforced accesses.

I'm not sure to what degree AccessedStorage::isUniquelyIdentified is used throughout the optimizer. I wouldn't be supprised if this PR doesn't have any meaningful performance gains. I think another pass that just updated accesses of "unique" references might be much more beneficial (performance-wise). We'll see.

Later, I'll clean up these individual commits and break each one into its own PR. The tests do not currently pass, this PR is just for benchmarking.

Refs (and based on) #32844. Related forum post.

Adds a new flag to alloc_ref instructions. This flag, "unique",
signifies that the reference is unique meaning it is not stored
elsewhere and may not be written to elsewhere. This means accesses of
this reference may be "static" instead of "dynamic".

Uniqueness of the reference is enforced by the SILVerifier. This is good
because it may be run between each pass so errors will be easy to track
down.

Currently the SILVerifier only supports a very few cases where it can
prove uniqueness, all other cases will end in an error. Later this
verifier check may be extended to be more robust.

Currently the only way to mark a reference as unique is through textual
SIL. Later optimization passes may be added or updated to add this
information.
…erence information and update AccessEnforcementOpts to promote accesses of unique storage to static access enforcement.
@zoecarver
Copy link
Contributor Author

@swift-ci please benchmark.

@swift-ci
Copy link
Contributor

Build failed before running benchmark.

 * Recurrsive functions / BB
 * Empty functions
@zoecarver
Copy link
Contributor Author

@swift-ci please benchmark.

@zoecarver
Copy link
Contributor Author

@swift-ci please benchmark

@swift-ci
Copy link
Contributor

Performance: -O

Improvement OLD NEW DELTA RATIO
NSStringConversion.MutableCopy.Rebridge.UTF8 822 742 -9.7% 1.11x (?)
NSStringConversion.MutableCopy.UTF8 975 901 -7.6% 1.08x (?)
StringBuilderLong 1530 1420 -7.2% 1.08x (?)
StringBuilderWithLongSubstring 1790 1670 -6.7% 1.07x (?)

Code size: -O

Improvement OLD NEW DELTA RATIO
Array2D.o 3094 3062 -1.0% 1.01x

Performance: -Osize

Improvement OLD NEW DELTA RATIO
String.data.LargeUnicode 119 108 -9.2% 1.10x (?)

Code size: -Osize

Improvement OLD NEW DELTA RATIO
LinkedList.o 2257 2200 -2.5% 1.03x
ArrayOfGenericRef.o 8304 8096 -2.5% 1.03x
ArrayOfRef.o 8581 8389 -2.2% 1.02x
OpaqueConsumingUsers.o 2351 2303 -2.0% 1.02x
RGBHistogram.o 19552 19216 -1.7% 1.02x
DictionaryBridge.o 3309 3269 -1.2% 1.01x
Array2D.o 2696 2664 -1.2% 1.01x

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

@shahmishal
Copy link
Member

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

  • How to change the base branch: (Link)
  • More detail about the branch update: (Link)

@zoecarver zoecarver closed this Oct 2, 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