Skip to content

[CopyPropagation] Canonicalize all values. #41943

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

Merged

Conversation

nate-chandler
Copy link
Contributor

Previously, the lifetimes of only values which were copied were canonicalized during the non-mandatory CopyPropagation pass. The mandatory pass (i.e. MandatoryCopyPropagation) has been disabled, meaning that non-copied values lifetimes were never canonicalized. So enable canonicalization of all lifetimes during regular CopyPropagation.

rdar://54335055

@nate-chandler nate-chandler requested a review from atrick March 21, 2022 22:57
@nate-chandler
Copy link
Contributor Author

@swift-ci please benchmark

@nate-chandler
Copy link
Contributor Author

------- Performance (x86_64): -O -------

REGRESSION                                     OLD     NEW    DELTA    RATIO
PrefixWhileArrayLazy                           20      26     +30.0%   **0.77x (?)**
DifferentiationIdentity                        247     320    +29.6%   **0.77x (?)**
DifferentiationSquare                          444     537    +20.9%   **0.83x (?)**
PrefixWhileAnySeqCntRange                      114     129    +13.2%   **0.88x (?)**
RemoveWhereMoveInts                            17      19     +11.8%   **0.89x (?)**
Data.hash.Empty                                47      52     +10.6%   **0.90x (?)**

IMPROVEMENT                                    OLD     NEW    DELTA    RATIO
SortArrayInClass                               14976   311    -97.9%   **48.15x**
DropWhileArrayLazy                             49      31     -36.7%   **1.58x**
FlattenListLoop                                1379    939    -31.9%   **1.47x (?)**
FlattenListFlatMap                             4234    3004   -29.1%   **1.41x (?)**
NSArray.nonbridged.mutableCopy.objectAtIndex   578     431    -25.4%   **1.34x (?)**
NSArray.nonbridged.objectAtIndex               560     424    -24.3%   **1.32x (?)**
PrefixWhileSequenceLazy                        26      20     -23.1%   **1.30x (?)**
DropWhileAnyCollectionLazy                     49      38     -22.4%   **1.29x**
NSArray.bridged.objectAtIndex                  278     216    -22.3%   **1.29x**
NSArray.bridged.mutableCopy.objectAtIndex      733     615    -16.1%   **1.19x (?)**
DropWhileAnySeqCRangeIterLazy                  45      38     -15.6%   **1.18x (?)**
DropWhileAnySeqCntRangeLazy                    45      38     -15.6%   **1.18x (?)**
DictionarySubscriptDefaultMutation             172     148    -14.0%   **1.16x (?)**
DropWhileSequenceLazy                          67      62     -7.5%    **1.08x (?)**
PrefixArray                                    14      13     -7.1%    **1.08x (?)**
Diffing.Similar                                373     347    -7.0%    **1.07x (?)**
PrefixWhileAnySequence                         198     185    -6.6%    **1.07x (?)**

------- Code size: -O -------

IMPROVEMENT                    OLD     NEW     DELTA    RATIO
Differentiation.o              7695    6063    -21.2%   **1.27x**
SortArrayInClass.o             2708    2612    -3.5%    **1.04x**
DictionarySubscriptDefault.o   19090   18770   -1.7%    **1.02x**

------- Performance (x86_64): -Osize -------

REGRESSION                                     OLD     NEW    DELTA    RATIO
DropWhileCountableRangeLazy                    49      71     +44.9%   **0.69x (?)**
DifferentiationIdentity                        253     332    +31.2%   **0.76x (?)**
DropWhileAnySeqCRangeIterLazy                  127     166    +30.7%   **0.77x (?)**
DropWhileAnySeqCntRangeLazy                    127     166    +30.7%   **0.77x (?)**
PrefixWhileAnyCollection                       111     144    +29.7%   **0.77x (?)**
Set.filter.Int100.28k                          37      47     +27.0%   **0.79x (?)**
SequenceAlgosRange                             1740    2170   +24.7%   **0.80x (?)**
Set.filter.Int100.16k                          21      26     +23.8%   **0.81x (?)**
Set.filter.Int100.20k                          26      32     +23.1%   **0.81x (?)**
Set.filter.Int100.24k                          31      38     +22.6%   **0.82x (?)**
DifferentiationSquare                          443     541    +22.1%   **0.82x**
SequenceAlgosArray                             1860    2180   +17.2%   **0.85x (?)**
ObjectiveCBridgeStubDateAccess                 130     152    +16.9%   **0.86x (?)**
PrefixWhileAnyCollectionLazy                   94      107    +13.8%   **0.88x (?)**
PrefixWhileAnySequence                         188     213    +13.3%   **0.88x (?)**
RemoveWhereMoveInts                            19      21     +10.5%   **0.90x (?)**
RemoveWhereQuadraticString                     186     201    +8.1%    **0.93x (?)**
LineSink.scalars.complex                       137     148    +8.0%    **0.93x (?)**

IMPROVEMENT                                    OLD     NEW    DELTA    RATIO
SortArrayInClass                               15158   403    -97.3%   **37.61x**
PrefixWhileCountableRangeLazy                  40      21     -47.5%   **1.90x (?)**
PrefixWhileSequenceLazy                        40      26     -35.0%   **1.54x**
DropWhileSequenceLazy                          71      49     -31.0%   **1.45x**
NSArray.nonbridged.objectAtIndex               568     424    -25.4%   **1.34x (?)**
NSArray.nonbridged.mutableCopy.objectAtIndex   577     431    -25.3%   **1.34x (?)**
RecursiveOwnedParameter                        327     251    -23.2%   **1.30x (?)**
PrefixWhileArrayLazy                           26      20     -23.1%   **1.30x (?)**
NSArray.bridged.objectAtIndex                  278     216    -22.3%   **1.29x**
NSStringConversion.UTF8                        718     592    -17.5%   **1.21x (?)**
NSArray.bridged.mutableCopy.objectAtIndex      733     619    -15.6%   **1.18x (?)**
NSStringConversion.Medium                      354     299    -15.5%   **1.18x (?)**
NSStringConversion.InlineBuffer.ASCII          293     257    -12.3%   **1.14x (?)**
SequenceAlgosContiguousArray                   1920    1710   -10.9%   **1.12x (?)**
Set.isSuperset.Seq.Empty.Int                   45      41     -8.9%    **1.10x (?)**
Set.isSubset.Int.Empty                         45      41     -8.9%    **1.10x (?)**
NSStringConversion.Long                        708     649    -8.3%    **1.09x (?)**
StringWalk                                     3520    3240   -8.0%    **1.09x (?)**
PrefixWhileAnySequenceLazy                     1179    1086   -7.9%    **1.09x (?)**
PrefixAnySequenceLazy                          1192    1112   -6.7%    **1.07x (?)**

------- Code size: -Osize -------

REGRESSION           OLD     NEW     DELTA    RATIO
Hash.o               18682   18997   +1.7%    **0.98x**

IMPROVEMENT          OLD     NEW     DELTA    RATIO
Differentiation.o    7225    5881    -18.6%   **1.23x**
SortArrayInClass.o   2956    2876    -2.7%    **1.03x**

------- Performance (x86_64): -Onone -------

REGRESSION                                        OLD     NEW    DELTA    RATIO
NSStringConversion.InlineBuffer.ASCII             5890    6705   +13.8%   **0.88x (?)**
NSStringConversion.InlineBuffer.UTF8              3606    4075   +13.0%   **0.88x (?)**

IMPROVEMENT                                       OLD     NEW    DELTA    RATIO
CharIteration_punctuatedJapanese_unicodeScalars   10360   9680   -6.6%    **1.07x (?)**

------- Code size: -swiftlibs -------

@atrick
Copy link
Contributor

atrick commented Mar 22, 2022

We're really starting to see the benefits of ownership representation now
SortArrayInClass 14976 311 -97.9% **48.15x**

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.

Good. This means the lifetime of owned values will be consistently canonicalized, within the new boundaries of lexical lifetimes.

@nate-chandler nate-chandler force-pushed the copy_propagation/canonicalize-all branch 5 times, most recently from cd76eb3 to 6b06ce2 Compare March 23, 2022 03:47
@nate-chandler
Copy link
Contributor Author

@swift-ci please benchmark

@nate-chandler
Copy link
Contributor Author

@swift-ci please test

@nate-chandler
Copy link
Contributor Author

@swift-ci please benchmark

@nate-chandler
Copy link
Contributor Author

@swift-ci please test macos platform

@nate-chandler
Copy link
Contributor Author

@swift-ci please test linux platform

1 similar comment
@nate-chandler
Copy link
Contributor Author

@swift-ci please test linux platform

@nate-chandler
Copy link
Contributor Author

@swift-ci please clean test macos platform

@nate-chandler
Copy link
Contributor Author

@swift-ci please clean test linux platform

@atrick
Copy link
Contributor

atrick commented Mar 23, 2022

Thanks for fixing the outliner to handle consuming arguments. It's good to have this pass working well with OSSA!

@nate-chandler
Copy link
Contributor Author

@swift-ci please clean test linux platform

@atrick
Copy link
Contributor

atrick commented Apr 13, 2022

@nate-chandler I just realized this never landed. Anything holding it up?

@nate-chandler
Copy link
Contributor Author

nate-chandler commented Apr 13, 2022

There were some docc test failures earlier. Let's see if those were spurious.

@nate-chandler
Copy link
Contributor Author

@swift-ci please test

@nate-chandler nate-chandler force-pushed the copy_propagation/canonicalize-all branch from 6b06ce2 to aaf4750 Compare May 25, 2022 18:46
@nate-chandler
Copy link
Contributor Author

@swift-ci please test

@nate-chandler nate-chandler force-pushed the copy_propagation/canonicalize-all branch from aaf4750 to 899141e Compare June 3, 2022 05:26
@nate-chandler
Copy link
Contributor Author

@swift-ci please test

The new utility looks through operands whose values are themselves
SILPhiArguments and visits those arguments' operands.
@nate-chandler nate-chandler force-pushed the copy_propagation/canonicalize-all branch from 899141e to 6fcd1f6 Compare July 19, 2022 03:14
@nate-chandler
Copy link
Contributor Author

@swift-ci please test

@nate-chandler nate-chandler force-pushed the copy_propagation/canonicalize-all branch from 6fcd1f6 to 228232f Compare July 20, 2022 22:01
@nate-chandler
Copy link
Contributor Author

@swift-ci please test

The new utility, given an phi, visits all adjacent phis (i.e. arguments
to the same block) which are (potentially iterated) reborrows of a value
reaching the given phi.
When canonicalizing lifetime of an owned phi, add adjacent phis which
are reborrows of its reaching values to the worklist.  Update liveness
to consider uses whose operand ownership is specific to guaranteed
values (i.e. uses which are derived from these adjacent reborrow phis)
as standard, non-lifetime-ending uses.

rdar://94346482
Properly handle the different sorts of reborrows when they are visited:
- non-branch reborrows (tuples, etc) are non-lifetime-ending uses; such
  reborrows are relevant defs for liveness
- branch reborrows which also consume the owned phi are lifetime-ending
  uses
- branch reborrows which DO NOT consume the owned phi are
  non-lifetime-ending uses, and the corresponding phis are relevant defs
  for liveness

rdar://97399065
@nate-chandler nate-chandler force-pushed the copy_propagation/canonicalize-all branch from 228232f to 7feccbd Compare July 21, 2022 22:47
@nate-chandler
Copy link
Contributor Author

@swift-ci please test

Previously, the match failed to find the larger sequence that began with
load [copy] and ended with destroy_value because the iterator advanced
after finding the load [copy].  Advanced the iterator here.

Enables reverting the test outlining test changes introduced in
a52b896.
Previously, the lifetimes of only values which were copied were
canonicalized during the non-mandatory CopyPropagation pass.  The
mandatory pass (i.e. MandatoryCopyPropagation) has been disabled,
meaning that non-copied values lifetimes were  never canonicalized.  So
enable canonicalization of all lifetimes during regular CopyPropagation.

rdar://54335055
Thanks to CopyPropagation canonicalizing all values, not just those for
which there is a copy_value instruction, the lifetime of the value that
is loaded in the BridgedProperty pattern is shortened and the
destroy_value appears right after its use rather than after the full CFG
for the bridged property.  Updated the BridgedProperty pattern to match
that newly hoisted instruction.
Put off calculating the outlined function name until the point at which
we will have all the inputs for the name.
Previously, for the BridgedProperty pattern, there were two flavors of
outlined function that would be formed, varying on the ownership of the
instance whose field is being access:

(1) guaranteed_in -- for use by addresses
(2) unowned -- for use by values which were not destroyed until some
              time after the pattern of interest is matched

Now that the lifetime of the instance may be shortened to just after the
call to the objc method, the latter of these is not appropriate for
values.  We would have to re-extend the lifetime until after the method
returns.

Instead, here, we add a new variant

(3) owned -- for use by values which were destroyed just after the objc
             method call

Doing so requires new mangling.
@nate-chandler nate-chandler force-pushed the copy_propagation/canonicalize-all branch from 7feccbd to a903805 Compare July 22, 2022 15:21
@nate-chandler
Copy link
Contributor Author

@swift-ci please test

@nate-chandler
Copy link
Contributor Author

@swift-ci please test source compatibility

@nate-chandler nate-chandler merged commit dcea147 into swiftlang:main Jul 22, 2022
@nate-chandler nate-chandler deleted the copy_propagation/canonicalize-all branch July 22, 2022 23:54
@meg-gupta meg-gupta modified the milestone: Swift 5.7 Sep 14, 2022
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