Skip to content

[ownership] Fix the eager specializer for ownership #32756

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
gottesmm merged 2 commits into from
Jul 10, 2020
Merged

[ownership] Fix the eager specializer for ownership #32756

gottesmm merged 2 commits into from
Jul 10, 2020

Conversation

@gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Jul 8, 2020

This is an in progress PR to fix the open coded part of the eager specializer for ownership now that the generic specializer itself is now fixed for OSSA.

@gottesmm gottesmm requested a review from atrick July 8, 2020 03:38
@gottesmm gottesmm force-pushed the eager_specializer_fix branch 3 times, most recently from 66daf86 to 69fec4f Compare July 9, 2020 06:39
@gottesmm gottesmm marked this pull request as ready for review July 9, 2020 06:39
@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 9, 2020

@swift-ci test

@gottesmm gottesmm changed the title [DRAFT][ownership] Fix the eager specializer for ownership [ownership] Fix the eager specializer for ownership Jul 9, 2020
@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 9, 2020

@swift-ci test source compatibility

@swift-ci
Copy link
Contributor

swift-ci commented Jul 9, 2020

Build failed
Swift Test Linux Platform
Git Sha - 69fec4f7dca330112a5387960d4d24325642c34b

@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 9, 2020

@swift-ci test linux platform

@swift-ci
Copy link
Contributor

swift-ci commented Jul 9, 2020

Build failed
Swift Test Linux Platform
Git Sha - 69fec4f7dca330112a5387960d4d24325642c34b

@swift-ci
Copy link
Contributor

swift-ci commented Jul 9, 2020

Build failed
Swift Test OS X Platform
Git Sha - 69fec4f7dca330112a5387960d4d24325642c34b

atrick
atrick reviewed Jul 9, 2020
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.

Look pretty good. I don't see test cases for

  • the load_borrow code path
  • the code path for a specializing a loadable, nontrivial generic type, where the members don't depend on the generic parameter

@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 9, 2020

Those test cases are there. I just didn't put in explicit FileCheck checks. I'll add them.

@gottesmm
[sil] Add a forwarding cast called unchecked_value_cast.
5e36ae1 
Today unchecked_bitwise_cast returns a value with ObjCUnowned ownership. This is
important to do since the instruction can truncate memory meaning we want to
treat it as a new object that must be copied before use.

This means that in OSSA we do not have a purely ossa forwarding unchecked
layout-compatible assuming cast. This role is filled by unchecked_value_cast.
@gottesmm gottesmm force-pushed the eager_specializer_fix branch from 69fec4f to 7a32079 Compare July 10, 2020 04:14
@gottesmm
[eager-specializer] Fix for ownership and add a bunch of missing code…
ac3109a 
… coverage for ossa.

Specifically, we were missing a bunch of coverage around specializing guaranteed
parameters and non-trivial values in general.
@gottesmm gottesmm force-pushed the eager_specializer_fix branch from 7a32079 to ac3109a Compare July 10, 2020 04:46
@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 10, 2020

@atrick I made it more explicit.

@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 10, 2020

@swift-ci test

2 similar comments
@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 10, 2020

@swift-ci test

@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 10, 2020

@swift-ci test

@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 10, 2020

@swift-ci test source compatibility

2 similar comments
@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 10, 2020

@swift-ci test source compatibility

@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 10, 2020

@swift-ci test source compatibility

@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 10, 2020

@swift-ci test

@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 10, 2020

@swift-ci test source compatibility

@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 10, 2020

@swift-ci benchmark

@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 10, 2020

@swift-ci test windows platform

@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 10, 2020

@swift-ci benchmark

@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 10, 2020

@swift-ci test windows platform

@swift-ci
Copy link
Contributor

swift-ci commented Jul 10, 2020

Performance: -O

Improvement OLD NEW DELTA RATIO
StringBuilderLong 1520 1420 -6.6% 1.07x (?)

Code size: -O

Performance: -Osize

Regression OLD NEW DELTA RATIO
FlattenListLoop 1633 2465 +50.9% 0.66x (?)
StringBuilderWithLongSubstring 1760 1980 +12.5% 0.89x (?)

Code size: -Osize

Performance: -Onone

Regression OLD NEW DELTA RATIO
ObjectiveCBridgeStubFromNSDate 5840 6780 +16.1% 0.86x (?)
 
Improvement OLD NEW DELTA RATIO
NSStringConversion.Mutable 2520 2298 -8.8% 1.10x (?)
String.data.LargeUnicode 138 126 -8.7% 1.10x (?)
DictionaryBridgeToObjC_Access 992 923 -7.0% 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 Jul 10, 2020

Build failed
Swift Test Linux Platform
Git Sha - ac3109a

@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 10, 2020

@swift-ci smoke test linux platform

@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 10, 2020

Compatibility failures I think are due to a swiftpm issue. The full linux test is failing in lldb in a way that can't be caused by this patch (I am not even running this in the pipeline yet).

@gottesmm gottesmm merged commit adec755 into swiftlang:master Jul 10, 2020
@gottesmm gottesmm deleted the eager_specializer_fix branch July 10, 2020 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@atrick atrick atrick left review comments

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