[sil] Ban casting AnyObject with a guaranteed ownership forwarding checked_cast_br and fix up semantic-arc-opts given that. #40287

gottesmm · 2021-11-23T20:14:46Z

The reason why I am doing this is that currently checked_cast_br of an AnyObject
may return a different value due to boxing. As an example, this can occur when
performing a checked_cast_br of a __SwiftValue(AnyHashable(Klass())).

To model this in SIL, I added to OwnershipForwardingMixin a bit of information
that states if the instruction directly forwards ownership with a default value
of true. In checked_cast_br's constructor though I specialize this behavior if
the source type is AnyObject and thus mark the checked_cast_br as not directly
forwarding its operand. If an OwnershipForwardingMixin directly forwards
ownership, one can assume that if it forwards ownership, it will always do so in
a way that ensures that each forwarded value is rc-identical to whatever result
it algebraically forwards ownership to. So in the context of checked_cast_br, it
means that the source value is rc-identical to the argument of the success and
default blocks.

I added a verifier check to CheckedCastBr that makes sure that it can never
forward guaranteed ownership and have a source type of AnyObject.

In SemanticARCOpts, I modified the code that builds extended live ranges of
owned values (*) to check if a OwnershipForwardingMixin is directly
forwarding. If it is not directly forwarding, then we treat the use just as an
unknown consuming use. This will then prevent us from converting such an owned
value to guaranteed appropriately in such a case.

(*) For those unaware, SemanticARCOpts contains a model of an owned value and
all forwarding uses of it called an OwnershipLiveRange.

rdar://85710101

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

gottesmm · 2021-11-23T20:16:22Z

@swift-ci smoke test

gottesmm · 2021-11-25T06:16:31Z

Turns out libswift hit the verification. I put a fix in SILGenBuilder to ensure that if we create a checked_cast_br, we ensurePlusOne if the source type is AnyObject.

gottesmm · 2021-11-25T07:00:44Z

@swift-ci test

gottesmm · 2021-11-25T09:08:52Z

@swift-ci test source compatibility

gottesmm · 2021-11-25T09:42:40Z

@swift-ci benchmark

swift-ci · 2021-11-25T10:40:23Z

Performance (x86_64): -O

Regression	OLD	NEW	DELTA	RATIO
DataAppendDataSmallToSmall	3700	4140	+11.9%	0.89x (?)

Improvement	OLD	NEW	DELTA	RATIO
NSStringConversion.MutableCopy.Rebridge.UTF8	862	750	-13.0%	1.15x (?)
String.data.Medium	108	100	-7.4%	1.08x (?)
EqualSubstringSubstring	42	39	-7.1%	1.08x (?)
EqualStringSubstring	42	39	-7.1%	1.08x (?)
EqualSubstringString	42	39	-7.1%	1.08x (?)
LessSubstringSubstring	43	40	-7.0%	1.07x (?)
EqualSubstringSubstringGenericEquatable	43	40	-7.0%	1.07x (?)
LessSubstringSubstringGenericComparable	43	40	-7.0%	1.07x (?)

Code size: -O

Improvement	OLD	NEW	DELTA	RATIO
AngryPhonebook.o	8983	8843	-1.6%	1.02x

Performance (x86_64): -Osize

Improvement	OLD	NEW	DELTA	RATIO
DictionaryBridgeToObjC_Access	991	882	-11.0%	1.12x (?)
LessSubstringSubstring	44	40	-9.1%	1.10x (?)
ObjectiveCBridgeStringIsEqualAllSwift	105	97	-7.6%	1.08x (?)
EqualSubstringSubstring	43	40	-7.0%	1.07x (?)
EqualStringSubstring	43	40	-7.0%	1.07x (?)
EqualSubstringSubstringGenericEquatable	43	40	-7.0%	1.07x (?)
EqualSubstringString	43	40	-7.0%	1.07x (?)
LessSubstringSubstringGenericComparable	43	40	-7.0%	1.07x (?)
StringBuilderWithLongSubstring	1580	1470	-7.0%	1.07x (?)
FlattenListFlatMap	6926	6457	-6.8%	1.07x (?)

Code size: -Osize

Performance (x86_64): -Onone

Regression	OLD	NEW	DELTA	RATIO
FloatingPointPrinting_Float_interpolated	65000	70800	+8.9%	0.92x (?)

Improvement	OLD	NEW	DELTA	RATIO
StringBuilderWithLongSubstring	4470	3740	-16.3%	1.20x (?)
DataToStringSmall	5050	4350	-13.9%	1.16x (?)
String.data.Medium	185	170	-8.1%	1.09x (?)

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

Thanks. Look great except...

swift::findOwnershipReferenceAggregate needs to be fixed to check for direct forwarding. Maybe there are other places?

The canOpcodeForwardGuaranteedValues APIs are the problem. They're completely misleading. Can they be fixed to be canDirectlyForwardValue and canDirectlyForwardOperand? That would be much better than adding a check to OwnershipLiveRange

I probably would have opted for custom logic in isDirectlyForwarding that checks for checked_cast_br, then calls CheckedCastBranch::isDirectlyForwarding, but the flag is cleaner so that's up to your judgement.

…ecked_cast_br and fix up semantic-arc-opts given that. The reason why I am doing this is that currently checked_cast_br of an AnyObject may return a different value due to boxing. As an example, this can occur when performing a checked_cast_br of a __SwiftValue(AnyHashable(Klass())). To model this in SIL, I added to OwnershipForwardingMixin a bit of information that states if the instruction directly forwards ownership with a default value of true. In checked_cast_br's constructor though I specialize this behavior if the source type is AnyObject and thus mark the checked_cast_br as not directly forwarding its operand. If an OwnershipForwardingMixin directly forwards ownership, one can assume that if it forwards ownership, it will always do so in a way that ensures that each forwarded value is rc-identical to whatever result it algebraically forwards ownership to. So in the context of checked_cast_br, it means that the source value is rc-identical to the argument of the success and default blocks. I added a verifier check to CheckedCastBr that makes sure that it can never forward guaranteed ownership and have a source type of AnyObject. In SemanticARCOpts, I modified the code that builds extended live ranges of owned values (*) to check if a OwnershipForwardingMixin is directly forwarding. If it is not directly forwarding, then we treat the use just as an unknown consuming use. This will then prevent us from converting such an owned value to guaranteed appropriately in such a case. I also in SILGen needed to change checked_cast_br emission in SILGenBuilder to perform an ensurePlusOne on the input operand (converting it to +1 with an appropriate cleanup) if the source type is an AnyObject. I found this via the verifier check catching this behavior from SILGen when compiling libswift. This just shows how important IR verification of invariants can be. (*) For those unaware, SemanticARCOpts contains a model of an owned value and all forwarding uses of it called an OwnershipLiveRange. rdar://85710101

gottesmm · 2021-11-30T00:08:53Z

@swift-ci smoke test

gottesmm · 2021-11-30T00:09:06Z

@swift-ci test

tbkka · 2021-11-30T00:20:14Z

I believe the concept here is correct: The optimization that assumes a successful cast of a reference type returns the same pointer does need to be avoided if the source is AnyObject, since AnyObject can hold references to various box types that can be unboxed to get at a contained class object reference.

tbkka

I believe the concept is sound; I'll let @atrick weigh in on the implementation.

swift-ci · 2021-11-30T02:05:12Z

Build failed
Swift Test OS X Platform
Git Sha - 8e5fb21

swift-ci · 2021-11-30T02:05:55Z

Build failed
Swift Test Linux Platform
Git Sha - 8e5fb21

gottesmm · 2021-11-30T02:06:14Z

I asked Mishal to kill the linux/macOS job since I am already running the smoke test.

atrick

Looks great!

atrick · 2021-11-30T05:18:17Z

lib/SIL/Utils/MemAccessUtils.cpp

+          if (ti->isDirectlyForwarding()) {
+            root = term->getOperand(0);
+            continue;
+          }


Not critical in this PR, but it seems that 6 lines above should now have an assert:

assert(cast<OwnershipForwardingTermInst>(root)->isDirectlyForwarding()); root = root->getDefiningInstruction()->getOperand(0);

gottesmm requested review from atrick and tbkka November 23, 2021 20:14

gottesmm force-pushed the pr-8bb19fcfe173f5e7b33ddd95dcbc5566be2262b8 branch from 4c9ab1c to 9862ae6 Compare November 25, 2021 07:00

gottesmm mentioned this pull request Nov 25, 2021

[move-operator] Start having SILGen emit lexical lifetimes and teach the optimizer how to maintain lexical lifetimes until the lexical lifetime elimination. #40201

Merged

atrick reviewed Nov 27, 2021

View reviewed changes

gottesmm force-pushed the pr-8bb19fcfe173f5e7b33ddd95dcbc5566be2262b8 branch from 9862ae6 to 8e5fb21 Compare November 30, 2021 00:08

tbkka approved these changes Nov 30, 2021

View reviewed changes

atrick approved these changes Nov 30, 2021

View reviewed changes

gottesmm merged commit 1ea21db into swiftlang:main Nov 30, 2021

gottesmm deleted the pr-8bb19fcfe173f5e7b33ddd95dcbc5566be2262b8 branch November 30, 2021 05:26

[sil] Ban casting AnyObject with a guaranteed ownership forwarding checked_cast_br and fix up semantic-arc-opts given that. #40287

[sil] Ban casting AnyObject with a guaranteed ownership forwarding checked_cast_br and fix up semantic-arc-opts given that. #40287

Uh oh!

Conversation

gottesmm commented Nov 23, 2021

Uh oh!

gottesmm commented Nov 23, 2021

Uh oh!

gottesmm commented Nov 25, 2021

Uh oh!

gottesmm commented Nov 25, 2021

Uh oh!

gottesmm commented Nov 25, 2021

Uh oh!

gottesmm commented Nov 25, 2021

Uh oh!

swift-ci commented Nov 25, 2021

Performance (x86_64): -O

Code size: -O

Performance (x86_64): -Osize

Code size: -Osize

Performance (x86_64): -Onone

Code size: -swiftlibs

Uh oh!

atrick left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Nov 30, 2021

Uh oh!

gottesmm commented Nov 30, 2021

Uh oh!

tbkka commented Nov 30, 2021

Uh oh!

tbkka left a comment

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Nov 30, 2021

Uh oh!

swift-ci commented Nov 30, 2021

Uh oh!

gottesmm commented Nov 30, 2021

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

atrick Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

atrick left a comment •

edited

Loading