Skip to content

Fix OSSA handling of address uses #39663

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 6 commits into from
Oct 13, 2021
Merged

Fix OSSA handling of address uses #39663

merged 6 commits into from
Oct 13, 2021

Conversation

atrick
Copy link
Contributor

@atrick atrick commented Oct 8, 2021

Fix code that analyzes address uses for use in OSSA utilities. Various correctness and efficiency issues noticed via code inspection.

This code was mostly disabled because of a bug. Fixing it exposes other bugs in OSSA utilities, so some of the logic is temporarily disabled. Those utilities have been fixed, but they are dependent on these changes so need to be in a following PR. Additional unit tests will be checked in with those changes.

@atrick
Copy link
Contributor Author

atrick commented Oct 8, 2021

@swift-ci test

@atrick atrick requested review from gottesmm, meg-gupta and nate-chandler and removed request for gottesmm October 8, 2021 22:01
@swift-ci
Copy link
Contributor

swift-ci commented Oct 8, 2021

Build failed
Swift Test Linux Platform
Git Sha - cb8f264af184802ab29fcf1e4a75b8e922ac3b1b

@swift-ci
Copy link
Contributor

swift-ci commented Oct 9, 2021

Build failed
Swift Test OS X Platform
Git Sha - cb8f264af184802ab29fcf1e4a75b8e922ac3b1b

@atrick
Copy link
Contributor Author

atrick commented Oct 9, 2021

Failures in
(watchsimulator-i386) :: api-digester/clang-module-dump.swift
(linux-x86_64).Concurrency/Runtime.async_task_priority_current.swift
(linux-x86_64).Sanitizers/tsan.racy_async_let_fibonacci.swift

@atrick
Copy link
Contributor Author

atrick commented Oct 9, 2021

@swift-ci test

@atrick
Copy link
Contributor Author

atrick commented Oct 9, 2021

@swift-ci test source compatibility

@atrick
Copy link
Contributor Author

atrick commented Oct 12, 2021

@swift-ci smoke test

@atrick
Copy link
Contributor Author

atrick commented Oct 12, 2021

@swift-ci benchmark

@swift-ci
Copy link
Contributor

Performance (x86_64): -O

Regression OLD NEW DELTA RATIO
RemoveWhereMoveInts 31 34 +9.7% 0.91x (?)
StringBuilderSmallReservingCapacity 315 342 +8.6% 0.92x (?)
InsertCharacterEndIndexNonASCII 59 64 +8.5% 0.92x (?)
StringBuilder 307 331 +7.8% 0.93x (?)
ObjectiveCBridgeStringGetASCIIContents 503 541 +7.6% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
FlattenListLoop 2537 1656 -34.7% 1.53x (?)
FlattenListFlatMap 7116 5681 -20.2% 1.25x (?)
StringFromLongWholeSubstring 5 4 -20.0% 1.25x (?)
Breadcrumbs.MutatedUTF16ToIdx.Mixed 309 279 -9.7% 1.11x (?)
LessSubstringSubstring 43 39 -9.3% 1.10x (?)
EqualSubstringSubstring 42 39 -7.1% 1.08x (?)
EqualStringSubstring 42 39 -7.1% 1.08x (?)
EqualSubstringString 42 39 -7.1% 1.08x (?)
EqualSubstringSubstringGenericEquatable 43 40 -7.0% 1.07x (?)
LessSubstringSubstringGenericComparable 43 40 -7.0% 1.07x (?)

Code size: -O

Performance (x86_64): -Osize

Regression OLD NEW DELTA RATIO
ObjectiveCBridgeStringHash 135 149 +10.4% 0.91x
RemoveWhereMoveInts 34 37 +8.8% 0.92x (?)
StringBuilderSmallReservingCapacity 315 342 +8.6% 0.92x
Array2D 6928 7520 +8.5% 0.92x (?)
InsertCharacterEndIndexNonASCII 59 64 +8.5% 0.92x (?)
RemoveWhereSwapInts 37 40 +8.1% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
ObjectiveCBridgeStubFromNSString 1782 1557 -12.6% 1.14x (?)
Breadcrumbs.MutatedIdxToUTF16.Mixed 315 285 -9.5% 1.11x (?)
LessSubstringSubstring 44 40 -9.1% 1.10x (?)
EqualSubstringSubstringGenericEquatable 44 40 -9.1% 1.10x (?)
ObjectiveCBridgeToNSDictionary 19600 18100 -7.7% 1.08x (?)
FlattenListFlatMap 6796 6291 -7.4% 1.08x (?)
EqualSubstringSubstring 43 40 -7.0% 1.07x (?)
EqualStringSubstring 43 40 -7.0% 1.07x (?)
EqualSubstringString 43 40 -7.0% 1.07x (?)
LessSubstringSubstringGenericComparable 43 40 -7.0% 1.07x (?)

Code size: -Osize

Performance (x86_64): -Onone

Regression OLD NEW DELTA RATIO
StringWordBuilderReservingCapacity 2570 2950 +14.8% 0.87x (?)
StringWordBuilder 2610 2990 +14.6% 0.87x (?)
ObjectiveCBridgeStringHash 135 150 +11.1% 0.90x (?)
 
Improvement OLD NEW DELTA RATIO
StringToDataMedium 7750 7000 -9.7% 1.11x (?)
Breadcrumbs.MutatedUTF16ToIdx.Mixed 317 287 -9.5% 1.10x (?)
StringToDataLargeUnicode 7800 7200 -7.7% 1.08x (?)

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

@atrick
Copy link
Contributor Author

atrick commented Oct 12, 2021

@swift-ci smoke test

atrick added 6 commits October 12, 2021 19:58
@atrick
Fix an obvious bug in InteriorPointerOperand::findTransitiveUses().
1678354 
The return value was inverted, meaning the OSSA utilities simply
bailed out in most cases.

Fixing this means potentially exposing other OSSA bugs.
@atrick
Fix findTransitiveUses to only record leaf uses.
a336bcd 
Recording uses is now optional. This utility is also used simply to
check for PointerEscape. When uses are recorded, they are used to
extend a live range. There could be a large number of transitive uses
that we don't want to pass back to the caller.
@atrick
Add a new test case in sil_combine_ossa.sil
ca0ad84 
To test finding transitive address uses
@atrick
Fix findTransitiveGuaranteedUses to only record leaf uses.
52e1fc2 
Recording uses is now optional. This utility is also used simply to
check for PointerEscape. When uses are recorded, they are used to
extend a live range. There could be a large number of transitive uses
that we don't want to pass back to the caller.
@atrick
Temporarily disable addresseses in findInnerGuaranteedUses
014b155 
Because not all OSSA utility fixes have been committed yet.
@atrick
Fix findInnerTransitiveUsesForAddress; add AddressUseKind
b4a47b6 
findInnerTransitiveUsesForAddress was incorrectly returning true for
pointer escapes.

Introduce enum AddressUseKind { NonEscaping, PointerEscape, Unknown };

Clients need to handle each of these cases differently.
@atrick
Copy link
Contributor Author

atrick commented Oct 13, 2021

@swift-ci test

@atrick
Copy link
Contributor Author

atrick commented Oct 13, 2021

@swift-ci test source compatibility

@atrick
Copy link
Contributor Author

atrick commented Oct 13, 2021

@swift-ci smoke test windows

@atrick
Copy link
Contributor Author

atrick commented Oct 13, 2021

Full SCK testing passed on this a few days ago. The current run passed through SwifterSwift. I need to merge this to start SCK testing on a dependent PR.

@atrick atrick merged commit ed6f701 into swiftlang:main Oct 13, 2021
@atrick atrick deleted the fix-addressuses branch October 13, 2021 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meg-gupta meg-gupta Awaiting requested review from meg-gupta

@nate-chandler nate-chandler Awaiting requested review from nate-chandler

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@atrick @swift-ci