OperandOwnership fixes required for CanonicalOSSA #35248

atrick · 2021-01-02T00:28:47Z

@gottesmm these are the fixes that were required by CanonicalOSSA and reviewed here:
#35241

These are mostly the same fixes that I put up a couple weeks ago, but they are simpler, more complete, and I've incorporated review feedback. You may run into the same issues.

A NonUse operand does not use the value itself, so it ignores ownership and does not require liveness. This is for operands that represent dependence on a type but are not actually passed the value of that type (e.g. they may refer an open_existential). This could be used for other dependence-only operands in the future. A TrivialUse operand has undefined ownership semantics aside from requiring liveness. Therefore it is only legal to pass the use a value with ownership None (a trivial value). Contrast this with things like InstantaneousUse or BitwiseEscape, which just don't care about ownership (i.e. they have no ownership semantics. All of the explicitly listed operations in this category require trivially typed operands. So the meaning is obvious to anyone adding SIL operations and updating OperandOwnership.cpp, without needing to decifer the value ownership kinds.

This is only used for OSSA verification currently, but will be a correctness issue when OSSA canonicalization is enabled.

Anything with a borrow scope in the caller needs to be considered a Borrow of its operand. The end_apply needs to be considered the lifetime-ending use of the borrow.

The begin_apply token represents the borrow scope of all owned and guaranteed call arguments. Although SILToken is (currently) trivially typed, it must have guaranteed ownership so end_apply and abort_apply will be recognized as lifetime-ending uses.

Now that OperandOwnership determines the operand constraints, it doesn't make sense to distinguish between Borrow and NestedBorrow at this level. We want these uses to automatically convert between the nested/non-nested state as the operand's ownership changes. The use does not need to impose any constraint on the ownership of the incoming value. For algorithms that need to distinguish nested borrows, it's still trivial to do so.

It's against the principles of pass design to check the driver mode within the pass. A pass always needs to do the same thing regardless of where it runs in the pass pipeline. It also needs to be possible to test passes in isolation.

Canonicalization should be able to convert a nested borrow to a borrow. In this case, the coroutine was a nested borrow and becomes a borrow.

It is illegal to borrow an unowned value. Unowned values are only allowed to have specific instantaneous uses. In this case, an unchecked conversion is valid because there is an assumption that within a destructor, self is guaranteed.

atrick · 2021-01-02T03:23:47Z

@swift-ci test

atrick · 2021-01-02T03:24:17Z

@swift-ci benchmark

atrick · 2021-01-02T03:24:27Z

@swift-ci test source compatibility

swift-ci · 2021-01-02T04:04:54Z

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
FlattenListFlatMap	3997	5542	+38.7%	0.72x (?)
Data.append.Sequence.809B.Count.RE.I	105	116	+10.5%	0.91x (?)
String.data.LargeUnicode	119	128	+7.6%	0.93x (?)

Improvement	OLD	NEW	DELTA	RATIO
UTF8Decode_InitFromBytes_ascii_as_ascii	500	456	-8.8%	1.10x (?)

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Improvement	OLD	NEW	DELTA	RATIO
SevenBoom	2137	1803	-15.6%	1.19x (?)

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 · 2021-01-02T04:54:19Z

Build failed
Swift Test OS X Platform
Git Sha - 07fccd9478c7172b49d167f8ad993c6c7a4a3ded

Borrowing an unowned value is illegal.

atrick · 2021-01-03T03:29:59Z

All tests, SCK, benchmarks pass. Rebasing to fix a CHECK line in IRGen/async/builtins.

atrick · 2021-01-03T03:30:51Z

@swift-ci test

swift-ci · 2021-01-03T04:37:34Z

Build failed
Swift Test Linux Platform
Git Sha - 24fa288

swift-ci · 2021-01-03T05:23:48Z

Build failed
Swift Test OS X Platform
Git Sha - 24fa288

atrick · 2021-01-03T06:34:33Z

@swift-ci test

swift-ci · 2021-01-03T07:35:58Z

Build failed
Swift Test OS X Platform
Git Sha - 24fa288

atrick · 2021-01-03T08:03:03Z

Back-to-back failures in the same LLDB test that I've seen failing recently in regular CI testing:

rdar://72774387 (Swift CI test failure: lldb-api :: lang/swift/optionset/TestSwiftOptionSetType.py; recursive_mutex lock failed)

atrick · 2021-01-04T19:22:05Z

@swift-ci test

atrick · 2021-01-04T19:25:23Z

@swift-ci smoke test

atrick · 2021-01-04T21:31:20Z

@swift-ci smoke test OS X Platform

atrick force-pushed the fix-operandownership branch from 02c9792 to df24d83 Compare January 2, 2021 02:31

atrick added 10 commits January 1, 2021 19:22

Fix OperandOwnership for convert_escape_to_noescape.

cdcd0bf

This is only used for OSSA verification currently, but will be a correctness issue when OSSA canonicalization is enabled.

Fix OperandOwnership for coroutines.

56f05da

Anything with a borrow scope in the caller needs to be considered a Borrow of its operand. The end_apply needs to be considered the lifetime-ending use of the borrow.

Give SILToken Guaranteed ownership.

07f5735

The begin_apply token represents the borrow scope of all owned and guaranteed call arguments. Although SILToken is (currently) trivially typed, it must have guaranteed ownership so end_apply and abort_apply will be recognized as lifetime-ending uses.

OperandOwnership Remove unnecessary special case for callee operand.

f4e0eae

Add an OptimizedMandatoryCombine pass variant.

cead6a5

It's against the principles of pass design to check the driver mode within the pass. A pass always needs to do the same thing regardless of where it runs in the pass pipeline. It also needs to be possible to test passes in isolation.

Add a begin_borrow canonicalization test case.

bc01d97

Canonicalization should be able to convert a nested borrow to a borrow. In this case, the coroutine was a nested borrow and becomes a borrow.

Comment OperandOwnership

93c884e

Fix OSSA in SILGenFunction::emitIVarDestroyer

5b37728

It is illegal to borrow an unowned value. Unowned values are only allowed to have specific instantaneous uses. In this case, an unchecked conversion is valid because there is an assumption that within a destructor, self is guaranteed.

atrick requested a review from gottesmm January 2, 2021 03:22

atrick force-pushed the fix-operandownership branch from df24d83 to 07fccd9 Compare January 2, 2021 03:23

atrick added 2 commits January 1, 2021 21:20

Fix a bunch of test cases with illegal OSSA

4678567

Borrowing an unowned value is illegal.

Comment OwnershipConstraints with respect to Unowned values.

24fa288

atrick force-pushed the fix-operandownership branch from 07fccd9 to 24fa288 Compare January 3, 2021 03:30

atrick merged commit 3a1c8fd into swiftlang:main Jan 4, 2021

atrick deleted the fix-operandownership branch October 19, 2022 00:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

OperandOwnership fixes required for CanonicalOSSA #35248

OperandOwnership fixes required for CanonicalOSSA #35248

Uh oh!

atrick commented Jan 2, 2021

Uh oh!

atrick commented Jan 2, 2021

Uh oh!

atrick commented Jan 2, 2021

Uh oh!

atrick commented Jan 2, 2021

Uh oh!

swift-ci commented Jan 2, 2021

Uh oh!

swift-ci commented Jan 2, 2021

Uh oh!

atrick commented Jan 3, 2021

Uh oh!

atrick commented Jan 3, 2021

Uh oh!

swift-ci commented Jan 3, 2021

Uh oh!

swift-ci commented Jan 3, 2021

Uh oh!

atrick commented Jan 3, 2021

Uh oh!

swift-ci commented Jan 3, 2021

Uh oh!

atrick commented Jan 3, 2021

Uh oh!

atrick commented Jan 4, 2021

Uh oh!

atrick commented Jan 4, 2021

Uh oh!

atrick commented Jan 4, 2021

Uh oh!

Uh oh!

OperandOwnership fixes required for CanonicalOSSA #35248

OperandOwnership fixes required for CanonicalOSSA #35248

Uh oh!

Conversation

atrick commented Jan 2, 2021

Uh oh!

atrick commented Jan 2, 2021

Uh oh!

atrick commented Jan 2, 2021

Uh oh!

atrick commented Jan 2, 2021

Uh oh!

swift-ci commented Jan 2, 2021

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

swift-ci commented Jan 2, 2021

Uh oh!

atrick commented Jan 3, 2021

Uh oh!

atrick commented Jan 3, 2021

Uh oh!

swift-ci commented Jan 3, 2021

Uh oh!

swift-ci commented Jan 3, 2021

Uh oh!

atrick commented Jan 3, 2021

Uh oh!

swift-ci commented Jan 3, 2021

Uh oh!

atrick commented Jan 3, 2021

Uh oh!

atrick commented Jan 4, 2021

Uh oh!

atrick commented Jan 4, 2021

Uh oh!

atrick commented Jan 4, 2021

Uh oh!

Uh oh!