Skip to content

OSSA support for Box types. AccessedStorage Box handling. isBorrowedAddress utility. #38797

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
merged 4 commits into from
Aug 9, 2021

Conversation

atrick
Copy link
Contributor

@atrick atrick commented Aug 7, 2021

These 4 commits are interdependent as far as avoiding test regressions. The two main features are

  • Unify Box/Class/Tail handling in AccessedStorage/MemAccessUtils.
  • Add isBorrowedAddress, which is required for OSSA-based transformations.

Add a tiny BorrowedAddress utility.

Determines whether an address might be inside a borrowed scope. If so,
then any address substitution needs to find that scope boundary to
avoid violating its basic guarantee that all uses are within scope.

MemAccessUtils: unify Box/Class/Tail storage for consistency and usability

It was originally convenient for exclusivity optimization to treat
boxes specially. We wanted to know that the 'Box' kind was always
uniquely identified. But that's not really important. And now that
AccessedStorage is being used more generally, the inconsistency is
problematic.

A consistent model is also must easier to understand and explain.

This also make the implementation of the utility simpler and more powerful.

Functional changes:

isRCIdentical will look through mark_dependence and mark_uninitialized.

findReferenceRoot is used consistently everywhere increasing analysis precision.

@atrick atrick requested review from meg-gupta and gottesmm August 7, 2021 18:39
@atrick
Copy link
Contributor Author

atrick commented Aug 7, 2021

The isBorrowedAddress utility will help fix #38672 (OSSA+TempRVO fix) and is required by #38470 (OSSA utilities)

@atrick atrick requested a review from nate-chandler August 7, 2021 18:42
@atrick
Copy link
Contributor Author

atrick commented Aug 7, 2021

@swift-ci test

@atrick
Copy link
Contributor Author

atrick commented Aug 7, 2021

@swift-ci test source compatibility

@atrick
Copy link
Contributor Author

atrick commented Aug 7, 2021

@swift-ci benchmark

@swift-ci
Copy link
Contributor

swift-ci commented Aug 7, 2021

Performance (x86_64): -O

Regression OLD NEW DELTA RATIO
DictionaryOfAnyHashableStrings_insert 3444 5586 +62.2% 0.62x
Set.isDisjoint.Int25 263 341 +29.7% 0.77x (?)
Set.isDisjoint.Int50 263 337 +28.1% 0.78x (?)
FlattenListFlatMap 4421 5135 +16.2% 0.86x (?)
DictionaryKeysContainsNative 22 25 +13.6% 0.88x (?)
ArrayAppendOptionals 2040 2300 +12.7% 0.89x (?)
DictionaryRemove 3170 3500 +10.4% 0.91x (?)
TypeName 904 980 +8.4% 0.92x (?)

Code size: -O

Performance (x86_64): -Osize

Improvement OLD NEW DELTA RATIO
ArrayAppendGenericStructs 2010 1330 -33.8% 1.51x (?)
FlattenListLoop 4864 3976 -18.3% 1.22x (?)

Code size: -Osize

Performance (x86_64): -Onone

Regression OLD NEW DELTA RATIO
NSStringConversion.Rebridge.Long 276 314 +13.8% 0.88x (?)
NSStringConversion.Rebridge.Medium 276 313 +13.4% 0.88x (?)
 
Improvement OLD NEW DELTA RATIO
String.data.Small 89 83 -6.7% 1.07x (?)

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

@swift-ci
Copy link
Contributor

swift-ci commented Aug 7, 2021

Build failed
Swift Test Linux Platform
Git Sha - f351cb4e8528e7a8f2005e0df4372a75575e575f

@swift-ci
Copy link
Contributor

swift-ci commented Aug 7, 2021

Build failed
Swift Test OS X Platform
Git Sha - f351cb4e8528e7a8f2005e0df4372a75575e575f

atrick added 4 commits August 7, 2021 15:26
to hasIdenticalStorage.

Be precise in preparation for unifying and clarifying the access base model.
InteriorPointer needs to handle all access base kinds that may be borrowed.
…ility

It was originally convenient for exclusivity optimization to treat
boxes specially. We wanted to know that the 'Box' kind was always
uniquely identified. But that's not really important. And now that
AccessedStorage is being used more generally, the inconsistency is
problematic.

A consistent model is also must easier to understand and explain.

This also make the implementation of the utility simpler and more powerful.

Functional changes:

isRCIdentical will look through mark_dependence and mark_uninitialized.

findReferenceRoot is used consistently everywhere increasing analysis precision.
Determines whether an address might be inside a borrowed scope. If so,
then any address substitution needs to find that scope boundary to
avoid violating its basic guarantee that all uses are within scope.
@atrick atrick force-pushed the ossa-isborrowedaddress branch from f351cb4 to a6cb545 Compare August 9, 2021 18:57
@atrick
Copy link
Contributor Author

atrick commented Aug 9, 2021

All SCK builds passed (although I don't know how to make them build with -sil-verify-all).

@atrick
Copy link
Contributor Author

atrick commented Aug 9, 2021

@swift-ci test

@meg-gupta
Copy link
Contributor

Thanks @atrick ! lgtm

@atrick atrick merged commit a93a1f8 into swiftlang:main Aug 9, 2021
@atrick atrick deleted the ossa-isborrowedaddress branch August 9, 2021 23:06
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