[semantic-arc] Change StorageGuaranteesLoadVisitor::visitClassAccess to find borrow scope introducers using utilities from OwnershipUtils.h. #30069

gottesmm · 2020-02-26T03:59:55Z

I added a new API into OwnershipUtils called
getSingleBorrowIntroducingValue. This API returns a single
BorrowScopeIntroducingValue for a passed in guaranteed value. If we can not find
such a BorrowScopeIntroducingValue or we find multiple such, we return None.

Using that, I refactored StorageGuaranteesLoadVisitor::visitClassAccess(...) to
use this new API which should make it significantly more robust since the
routine uses the definitions of "guaranteed forwarding" that the ownership
verifier uses when it verifies meaning that we can rely on the routine to be
exhaustive and correct. This means that we now promote load [copy] ->
load_borrow even if our borrow scope introducer feeds through a switch_enum or
checked_cast_br result (the main reason I looked into this change).

To create getSingleBorrowIntroducingValue, I refactored
getUnderlyingBorrowIntroucingValues to use a generator to find all of its
underlying values. Then in getSingleBorrowIntroducingValue, I just made it so
that we call the generator 1-2 times (as appropriate) to implement this query.

gottesmm · 2020-02-26T04:02:01Z

@atrick the biggest change in the patterns we support is not we can promote load [copy] -> load_borrow if our guaranteed base is switched or casted upon via switch_enums, checked_cast_br.

I.e. we can transform this:

switch_enum %0, bb1, bb2

bb1(%1 : @guaranteed $Klass):
   %2 = ref_element_addr %1 : $Klass, #Klass.aLetField
   %3 = load [copy] %2
   apply %f(%3)
   destroy_value %3
   ...

Into this:

switch_enum %0, bb1, bb2

bb1(%1 : @guaranteed $Klass):
   %2 = ref_element_addr %1 : $Klass, #Klass.aLetField
   %3 = load_borrow %2
   apply %f(%3)
   end_borrow %3
   ...

gottesmm · 2020-02-26T04:02:20Z

@swift-ci test

gottesmm · 2020-02-26T04:02:25Z

@swift-ci test source compatibility

gottesmm · 2020-02-26T04:02:30Z

@swift-ci benchmark

atrick

The changes to semantic ARC look correct to me. The new utility just doesn't make sense... because it's reason for existing is based entirely on some nonsense in the original implementation...

atrick · 2020-02-26T04:39:21Z

lib/SIL/OwnershipUtils.cpp

+
+BorrowScopeIntroducingValueGenerator::Result
+BorrowScopeIntroducingValueGenerator::next() {
+  while (!worklist.empty()) {


I don't like the idea of a worklist here because it contradicts the basic design of a single borrow introducer for each scope.

What if I have a struct literal that has multiple guaranteed parameters lets say from different sil function arguments.

In such a case, I think we would need to return both borrow introducers.

Ok, if we allow a borrowed tuple or struct to be composed from borrowed elements, then this utility makes sense. The only thing I don't like is the general code that places all operands on the worklist. I feel like forwarding instructions (casts) should have an API that returns the single forwarded operand. And tuple/struct should be handled explicitly as the only thing that can "merge" borrow scopes.

There should be no reason to allow cond_br to handle borrowed values. No passes require critical edges in the mandatory pipeline. If it's done temporarily until we get around to enforcing that, then it should be a FIXME.

atrick · 2020-02-26T04:39:59Z

lib/SIL/OwnershipUtils.cpp

+    // instruction
+    if (isGuaranteedForwardingValue(v)) {
+      if (auto *i = v->getDefiningInstruction()) {
+        llvm::transform(i->getAllOperands(), std::back_inserter(worklist),


Any forwarding instruction should identify the single operand that it forwards. We shouldn't just arbitrarily dump all the operands on a worklist.

It is nonsense to forward multiple guaranteed values into a single scope.

atrick · 2020-02-26T04:40:48Z

lib/SIL/OwnershipUtils.cpp

+      auto *arg = cast<SILPhiArgument>(v);
+      auto *termInst = arg->getSingleTerminator();
+      assert(termInst && termInst->isTransformationTerminator());
+      llvm::transform(termInst->getAllOperands(), std::back_inserter(worklist),


Some here, a forwarding terminator should identify the single operand that it forwards

I am fine with doing that.

atrick · 2020-02-26T04:41:10Z

lib/SIL/OwnershipUtils.cpp

+namespace {
+
+/// A generator of borrow scope introducing values.
+struct BorrowScopeIntroducingValueGenerator {


I don't think we want this abstraction at all.... see comments below.

swift-ci · 2020-02-26T05:37:57Z

Performance: -O

Code size: -O

Performance: -Osize

Regression	OLD	NEW	DELTA	RATIO
DataAppendDataSmallToSmall	3880	4340	+11.9%	0.89x (?)

Improvement	OLD	NEW	DELTA	RATIO
DictionaryBridgeToObjC_Access	952	837	-12.1%	1.14x (?)

Code size: -Osize

Performance: -Onone

Regression	OLD	NEW	DELTA	RATIO
DataAppendDataSmallToSmall	3940	4420	+12.2%	0.89x (?)

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

gottesmm · 2020-02-26T22:37:22Z

I am going to swing back around and clean this up a little bit/add some more comments/etc before this lands.

…to find borrow scope introducers using utilities from OwnershipUtils.h. I added a new API into OwnershipUtils called getSingleBorrowIntroducingValue. This API returns a single BorrowScopeIntroducingValue for a passed in guaranteed value. If we can not find such a BorrowScopeIntroducingValue or we find multiple such, we return None. Using that, I refactored StorageGuaranteesLoadVisitor::visitClassAccess(...) to use this new API which should make it significantly more robust since the routine uses the definitions of "guaranteed forwarding" that the ownership verifier uses when it verifies meaning that we can rely on the routine to be exhaustive and correct. This means that we now promote load [copy] -> load_borrow even if our borrow scope introducer feeds through a switch_enum or checked_cast_br result (the main reason I looked into this change). To create getSingleBorrowIntroducingValue, I refactored getUnderlyingBorrowIntroucingValues to use a generator to find all of its underlying values. Then in getSingleBorrowIntroducingValue, I just made it so that we call the generator 1-2 times (as appropriate) to implement this query.

gottesmm · 2020-03-02T08:56:14Z

@swift-ci smoke test

gottesmm · 2020-03-02T09:18:29Z

@swift-ci benchmark

gottesmm · 2020-03-02T09:18:33Z

@swift-ci benchmark

swift-ci · 2020-03-02T09:42:35Z

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
MapReduce	207	224	+8.2%	0.92x (?)

Improvement	OLD	NEW	DELTA	RATIO
PrefixArray	14	13	-7.1%	1.08x (?)

Code size: -O

Performance: -Osize

Regression	OLD	NEW	DELTA	RATIO
DropFirstCountableRange	16	18	+12.5%	0.89x (?)

Code size: -Osize

Performance: -Onone

Improvement	OLD	NEW	DELTA	RATIO
DataAppendDataSmallToSmall	3180	2960	-6.9%	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 mini
  Model Identifier: Macmini8,1
  Processor Name: Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 64 GB

gottesmm requested a review from atrick February 26, 2020 03:59

atrick approved these changes Feb 26, 2020

View reviewed changes

gottesmm force-pushed the pr-cdc5d32f1a5317b8716c016a0e72f9ee40eae4dd branch from 46efc9a to 962b4ed Compare March 2, 2020 08:56

gottesmm merged commit ae2bef9 into swiftlang:master Mar 3, 2020

gottesmm deleted the pr-cdc5d32f1a5317b8716c016a0e72f9ee40eae4dd branch March 3, 2020 06:58

[semantic-arc] Change StorageGuaranteesLoadVisitor::visitClassAccess to find borrow scope introducers using utilities from OwnershipUtils.h. #30069

[semantic-arc] Change StorageGuaranteesLoadVisitor::visitClassAccess to find borrow scope introducers using utilities from OwnershipUtils.h. #30069

Uh oh!

Conversation

gottesmm commented Feb 26, 2020

Uh oh!

gottesmm commented Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gottesmm commented Feb 26, 2020

Uh oh!

gottesmm commented Feb 26, 2020

Uh oh!

gottesmm commented Feb 26, 2020

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gottesmm Feb 26, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Feb 26, 2020

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

gottesmm commented Feb 26, 2020

Uh oh!

gottesmm commented Mar 2, 2020

Uh oh!

gottesmm commented Mar 2, 2020

Uh oh!

gottesmm commented Mar 2, 2020

Uh oh!

swift-ci commented Mar 2, 2020

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

Uh oh!

gottesmm commented Feb 26, 2020 •

edited

Loading

gottesmm Feb 26, 2020 •

edited

Loading