[sil-combine] Update sil combine for Ownership [part 1] #35246

gottesmm · 2020-12-31T22:23:06Z

No description provided.

gottesmm · 2021-01-01T19:16:47Z

gottesmm · 2021-01-01T19:45:43Z

Just an intermediate check that I didn't miss anything before I upload a bunch more patches.

atrick

I reviewed all 10 commits currently in the PR, through [sil-combine] Enable switch_value to be optimized in OSSA. a0ac0b3

It all looks great. Nothing needs to be revised, just a bit of commentary.

test/SILOptimizer/sil_combine_devirt_ossa.sil

atrick · 2021-01-02T23:55:35Z

lib/SILOptimizer/SILCombiner/SILCombinerMiscVisitors.cpp

@@ -805,6 +806,8 @@ static bool isZeroLoadFromEmptyCollection(LoadInst *LI) {
      case ValueKind::UpcastInst:
      case ValueKind::RawPointerToRefInst:
      case ValueKind::AddressToPointerInst:
+      case ValueKind::BeginBorrowInst:
+      case ValueKind::CopyValueInst:


fyi, this is just an example of one of the 100+ places we should use AccessedStorageVisitor so we're not maintaining them all

Sure. Maybe I can fix that in a subsequent commit.

atrick · 2021-01-03T00:17:12Z

lib/SILOptimizer/SILCombiner/SILCombinerMiscVisitors.cpp

-  LoadInst *EnumVal = Builder.createLoad(SEAI->getLoc(), Addr,
-                                         LoadOwnershipQualifier::Unqualified);
-  Builder.createSwitchEnum(SEAI->getLoc(), EnumVal, Default, Cases);
+  SILValue EnumVal = Builder.emitLoadBorrowOperation(SEAI->getLoc(), Addr);


It took me a while to figure out how this is safe. It would help to comment that this only creates a trivial load-borrow scope to cover the switch_enum terminator and its block argument results, so nothing can write to memory within this scope.

I'll add to that.

I looked, I do have a comment a line or two above:

// Promote switch_enum_addr to switch_enum if the enum is loadable. // switch_enum_addr %ptr : $*Optional<SomeClass>, case ... // -> // %value = load %ptr // switch_enum %value // // If we are using ownership, we perform a load_borrow right before the new // switch_enum and end the borrow scope right afterwards.

gottesmm · 2021-01-04T17:55:50Z

Going to fix the comments in post-commit and do more work in a different PR that builds on this.

gottesmm · 2021-01-04T17:55:56Z

@swift-ci smoke test

gottesmm · 2021-01-04T18:41:29Z

@atrick I actually fixed the issues your brought up in this PR since I wanted to make it so that my patch around ref_to_* used pointer escape instead of unowned instantaneous use as per our conversations.

gottesmm · 2021-01-04T18:41:34Z

@swift-ci smoke test

gottesmm · 2021-01-04T19:01:12Z

@swift-ci smoke test OS X platform

gottesmm · 2021-01-04T19:20:39Z

@swift-ci smoke test OS X platform

gottesmm · 2021-01-04T19:20:47Z

@shahmishal hitting weird failures.

gottesmm · 2021-01-04T19:22:06Z

lib/SIL/IR/OperandOwnership.cpp

+// operand perspective we treat them as a pointer escape and from a value
+// perspective, they return a value with OwnershipKind::Unowned.
+#define ALWAYS_OR_SOMETIMES_LOADABLE_CHECKED_REF_STORAGE(Name, ...)            \
+  OPERAND_OWNERSHIP(PointerEscape, RefTo##Name)                                \


@atrick this is what I was talking about.

gottesmm · 2021-01-04T19:35:49Z

@swift-ci smoke test OS X platform

gottesmm · 2021-01-04T20:05:27Z

@swift-ci smoke test OS X platform

gottesmm · 2021-01-04T21:07:20Z

@swift-ci clean smoke test linux platform

gottesmm · 2021-01-04T23:14:34Z

@swift-ci smoke test

gottesmm · 2021-01-04T23:14:46Z

Got a merge conflict from one of Andy's commits.

@example

…Unowned so should be treated as PointerEscapes. These instructions model a conversion in between ownership kinds without the result actually being owned by anything. As a result: 1. From an operand perspective, the instruction is treated like a pointer escape. 2. From a value perspective, the instruction returns a value with OwnershipKind::Unowned (to force the value to be copied before it can be used in an owned or guaranteed way) and Example: ``` sil @example : $@convention(thin) (@owned Klass) -> @owned @sil_unowned Klass { bb0(%0 : @owned $Klass): // Note that the ref_to_unowned does not consume %0 but instead converts %0 // from a "strong" value to a "safe unowned" value. A "safe unowned" value is // a value that corresponds to an 'unowned' value at the Swift level that use // unowned reference counting. At the SIL level these values can be recognized // by their types having the type attribute @sil_unowned. We have not // incremented the unowned ref count of %1 so we must treat %1 as unowned. %1 = ref_to_unowned %0 : $Klass // Then before we can use %2 in any way as a "safe unowned" value we need to // bump its unowned ref count by making a copy of the value. %2 will be a // "safe unowned" value with OwnershipKind::Owned ensuring that we decrement // the unowned ref count and do not leak said ref count. %2 = copy_value %1 : $@sil_unowned $Klass // Then since the original ref_to_unowned did not consume %0, we need to // destroy it here. destroy_value %0 : $Klass // And then return out OwnershipKind::Owned @sil_unowned Klass. return %2 : $@sil_unowned $Klass } ```

…neous use.

…ent upstreamed work.

…t I found.

…g the checks we don't pass yet. In the subsequent commits, I am going to enable individual groups of filecheck tests. This will let me avoid having to extract out individual test cases as I upstream. NOTE: I disabled the CHECK lines by just s/CHECK/XHECK/g. I left in the CHECK-LABEL so FileCheck still has something to chew on initially.

…e.sil. These are mostly handled by inst simplify and simple DCE functionality.

…upport LoadBorrow. NOTE: The stdlib count/capacity propagation code is tested in an end<->end fashion in a separate Swift test. Once I flip the switch, that test will run. The code is pretty simple, so I feel relatively confident with it.

…es for ownership.

This routine never touches values with non-trivial ownership, so I just removed the early exit and updated the style.

gottesmm · 2021-01-04T23:46:11Z

@swift-ci smoke test

gottesmm · 2021-01-05T01:09:08Z

@swift-ci smoke test linux platform

gottesmm · 2021-01-05T02:53:41Z

@swift-ci smoke test linux platform

atrick · 2021-01-05T17:46:06Z

@gottesmm are there any functional changes here? I'm guessing not because it was only smoke tested!

gottesmm requested a review from atrick December 31, 2020 22:23

gottesmm force-pushed the ossa-sil-combine-final branch from 812242e to 63d7b21 Compare January 1, 2021 00:36

atrick reviewed Jan 3, 2021

View reviewed changes

gottesmm changed the title ~~[sil-combine] Update sil combine for Ownership.~~ [sil-combine] Update sil combine for Ownership [part 1] Jan 4, 2021

gottesmm force-pushed the ossa-sil-combine-final branch from a0ac0b3 to 0ca8a98 Compare January 4, 2021 18:38

gottesmm commented Jan 4, 2021

View reviewed changes

gottesmm force-pushed the ossa-sil-combine-final branch from a97b0ce to 0471653 Compare January 4, 2021 23:14

gottesmm added 9 commits January 4, 2021 15:37

[ownership] bridge_object_to_word is a bitwise escape not an instanta…

f6c7c0d

…neous use.

[sil-combine] Add ossa test translations that already pass given curr…

eb254f7

…ent upstreamed work.

[sil-combine] Add a test for non-ossa to fix a test coverage hole tha…

e18243f

…t I found.

[sil-combine] Enable some checks that already pass in ossa_sil_combin…

c99874b

…e.sil. These are mostly handled by inst simplify and simple DCE functionality.

[sil-combine] Update switch_enum_addr -> switch_enum for loadable typ…

0a0f8cf

…es for ownership.

[sil-combine] Update select_enum_addr canonicalization to use OSSA.

2413f5f

gottesmm added 2 commits January 4, 2021 15:44

[sil-combine] Enable switch_value to be optimized in OSSA.

5604831

This routine never touches values with non-trivial ownership, so I just removed the early exit and updated the style.

Fix a comment in a test to be easier to read.

b206d00

gottesmm force-pushed the ossa-sil-combine-final branch from 0471653 to b206d00 Compare January 4, 2021 23:45

gottesmm merged commit 0787072 into swiftlang:main Jan 5, 2021

gottesmm deleted the ossa-sil-combine-final branch January 5, 2021 05:14

[sil-combine] Update sil combine for Ownership [part 1] #35246

[sil-combine] Update sil combine for Ownership [part 1] #35246

Uh oh!

Conversation

gottesmm commented Dec 31, 2020

Uh oh!

gottesmm commented Jan 1, 2021

Uh oh!

gottesmm commented Jan 1, 2021

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

atrick Jan 2, 2021

Choose a reason for hiding this comment

Uh oh!

gottesmm Jan 3, 2021

Choose a reason for hiding this comment

Uh oh!

atrick Jan 3, 2021

Choose a reason for hiding this comment

Uh oh!

gottesmm Jan 3, 2021

Choose a reason for hiding this comment

Uh oh!

gottesmm Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Jan 4, 2021

Uh oh!

gottesmm commented Jan 4, 2021

Uh oh!

gottesmm commented Jan 4, 2021

Uh oh!

gottesmm commented Jan 4, 2021

Uh oh!

gottesmm commented Jan 4, 2021

Uh oh!

gottesmm commented Jan 4, 2021

Uh oh!

gottesmm commented Jan 4, 2021

Uh oh!

gottesmm Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Jan 4, 2021

Uh oh!

gottesmm commented Jan 4, 2021

Uh oh!

gottesmm commented Jan 4, 2021

Uh oh!

gottesmm commented Jan 4, 2021

Uh oh!

gottesmm commented Jan 4, 2021

Uh oh!

gottesmm commented Jan 4, 2021

Uh oh!

gottesmm commented Jan 5, 2021

Uh oh!

gottesmm commented Jan 5, 2021

Uh oh!

atrick commented Jan 5, 2021

Uh oh!

Uh oh!