Skip to content

[sil-combine] Use canonicalizeOSSALifetimes to eliminate unnecessary copies inserted due to RAUWing #37208

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 4 commits into from
May 4, 2021
Merged

[sil-combine] Use canonicalizeOSSALifetimes to eliminate unnecessary copies inserted due to RAUWing #37208

merged 4 commits into from
May 4, 2021

Conversation

gottesmm
Copy link
Contributor

@gottesmm gottesmm commented May 1, 2021

This is the first in a series of patches that use canonicalizeOSSALifetimes to eliminate extra copies inserted. canonicalizeOSSALifetimes is a new helper that takes in a list of defs found via CanonicalizeOSSALifetime::getCanonicalCopiedDef(...) and canonicalizes them. I added it in SILCombine to canonicalize away any copies inserted due to us RAUWing values using the OwnershipRAUWHelper.

I am going to add it into other passes where we are creating new ownership calls (consider mem2reg). This will eliminate phase ordering issues in the compiler.

gottesmm added 4 commits May 1, 2021 15:58
@gottesmm
[sil-optimizer] Allow for clients of SILInstructionWorklist to add op…
4c9cd62 
…erands to the worklist without deleting instructions.

This is needed to allow for clients to compose InstModCallbacks with
SILInstructionWorklist callbacks.
@gottesmm
[copy-propagation] Finish plumbing through InstModCallbacks into one …
3ab207c 
…deletion utility that I missed.
@gottesmm
[ownership] Add a simple helper wrapper called canonicalizeOSSALifeti…
387806b 
…mes around CanonicalizeOSSALifetime that clean up extra copies inserted when RAUWing or promoting loads.

Based on code in CopyPropagation. It is assumed that the passed in set of defs
is unique and that all such defs were found by using
CanonicalizeOSSALifetime::getCanonicalCopiedDef(copy).
@gottesmm
[sil-combine] Canonicalize copies using canonicalizeOSSALifetimes whe…
e670d2b 
…n moving newly created instructions into the worklist.

To be more explicit, canonicalizeOSSALifetimes is a utility that
re-canonicalizes all at once a set of defs that the caller found by applying
CanonicalizeOSSALifetime::getCanonicalCopiedDef(copy)). The reason why I am
doing this is that when we RAUW in OSSA, we sometimes insert additional copies
to make the problem easier for a utility to handle. This lets us canonicalize
away any copies before we even leave the pass.
@gottesmm gottesmm requested a review from atrick May 1, 2021 23:19
@gottesmm
Copy link
Contributor Author

gottesmm commented May 1, 2021

@swift-ci test

@gottesmm
Copy link
Contributor Author

gottesmm commented May 1, 2021

@swift-ci benchmark

@swift-ci
Copy link
Contributor

swift-ci commented May 2, 2021

Performance: -O

Regression OLD NEW DELTA RATIO
FlattenListLoop 1630 2498 +53.3% 0.65x (?)
FlattenListFlatMap 3975 6073 +52.8% 0.65x (?)
NSStringConversion.MutableCopy.Rebridge 820 902 +10.0% 0.91x (?)
DataAppendDataSmallToSmall 4080 4480 +9.8% 0.91x (?)
 
Improvement OLD NEW DELTA RATIO
NSStringConversion.UTF8 1077 953 -11.5% 1.13x (?)
DictionaryBridgeToObjC_Access 937 834 -11.0% 1.12x (?)

Code size: -O

Performance: -Osize

Regression OLD NEW DELTA RATIO
UTF8Decode_InitFromBytes_ascii_as_ascii 471 554 +17.6% 0.85x (?)
UTF8Decode_InitFromData_ascii_as_ascii 713 783 +9.8% 0.91x (?)
 
Improvement OLD NEW DELTA RATIO
FlattenListFlatMap 6634 6140 -7.4% 1.08x (?)
ObjectiveCBridgeStubToNSStringRef 130 121 -6.9% 1.07x (?)

Code size: -Osize

Performance: -Onone

Improvement OLD NEW DELTA RATIO
String.data.Medium 194 165 -14.9% 1.18x (?)

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

@gottesmm
Copy link
Contributor Author

gottesmm commented May 2, 2021

@swift-ci benchmark

@gottesmm
Copy link
Contributor Author

gottesmm commented May 2, 2021

I am not expecting any benchmark changes. What I am trying to just do is teach individual passes that use the Ownership RAUW infrastructure how to use copy propagation to eliminate unnecessary copies by canonicalizing lifetimes.

@swift-ci
Copy link
Contributor

swift-ci commented May 2, 2021

Performance: -O

Regression OLD NEW DELTA RATIO
String.data.Medium 115 127 +10.4% 0.91x (?)
 
Improvement OLD NEW DELTA RATIO
FlattenListFlatMap 5849 5198 -11.1% 1.13x (?)

Code size: -O

Performance: -Osize

Regression OLD NEW DELTA RATIO
FlattenListFlatMap 4414 5367 +21.6% 0.82x (?)

Code size: -Osize

Performance: -Onone

Regression OLD NEW DELTA RATIO
String.data.Empty 72 82 +13.9% 0.88x (?)
 
Improvement OLD NEW DELTA RATIO
SevenBoom 1705 1422 -16.6% 1.20x (?)

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

@gottesmm
Copy link
Contributor Author

gottesmm commented May 2, 2021

Regressions are different. Its most likely noise.

atrick
atrick approved these changes May 3, 2021
View reviewed changes
Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

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

lgtm

lib/SILOptimizer/SILCombiner/SILCombine.cpp
// eliminate unnecessary copies introduced by RAUWing when ownership is
// enabled.
//
// NOTE: It is ok if copy propagation results in MadeChanges being set to
Copy link
Contributor

Choose a reason for hiding this comment

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

Someone reading this comment won't realize that CanonicalizeOSSALifetime is mainly doing copy propagation.

@gottesmm gottesmm merged commit 617a027 into swiftlang:main May 4, 2021
@gottesmm gottesmm deleted the canonicalize-ossa-lifetimes-sil-combine branch May 4, 2021 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atrick atrick atrick approved these changes

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
@gottesmm @swift-ci @atrick