[ownership] Loosen restrictions around what we specialize and add generic specialization tests behind a flag. #32397

gottesmm · 2020-06-15T23:06:18Z

The idea is that this will let me remove these assertions that were in place to
make sure we were really conservative around specializing ownership code. For me
to remove that I need to be able to actually test out this code (since I think
there are some code paths where this will trigger in other parts of the compiler
now).

So to work out the kinks, I added a flag that allows for the generic specializer
to process ownership code and translated most of the .sil test cases/fixed any
bugs that I found. This hopefully will expose anything that is missing.

NOTE: I have not enabled the generic specializer running in ownership in the
pipeline. This is just a step in that direction by adding tests/etc.

gottesmm · 2020-06-15T23:09:19Z

lib/SILOptimizer/Utils/Generics.cpp

+                                 ArrayRef<SILValue> values) {
+  for (SILValue v : values) {
+    if (auto *lbi = dyn_cast<LoadBorrowInst>(v)) {
+      builder.createEndBorrow(loc, lbi);


I am a little nervous about this. I think I am going to change values here to have a bit that says it needs an end borrow or something like that.

You are right: what if there was a load_borrow before specialization? The you would insert a wrong end_borrow

gottesmm · 2020-06-15T23:12:43Z

lib/SILOptimizer/Utils/Generics.cpp

+
+      // Look through copy_value.
+      if (auto *cvi = dyn_cast<CopyValueInst>(User)) {
+        llvm::copy(cvi->getUses(), std::back_inserter(worklist));


I added this b/c some of the tests around recursive specialization started failing b/c it couldn't look through these instructions/ignore incidental instructions.

eeckstein

basically lgtm.
Request for changes only for a better way to decide on inserting an end_borrow (see my comment)

eeckstein · 2020-06-16T06:38:12Z

lib/SILOptimizer/Utils/Generics.cpp

+                                 ArrayRef<SILValue> values) {
+  for (SILValue v : values) {
+    if (auto *lbi = dyn_cast<LoadBorrowInst>(v)) {
+      builder.createEndBorrow(loc, lbi);


You are right: what if there was a load_borrow before specialization? The you would insert a wrong end_borrow

gottesmm · 2020-06-16T18:57:06Z

@eeckstein I fixed it by using a SmallBitVector. I learned about something new that I didn't know about: SmallBitVector now has an iterator for set bit indices! So you can just do:

for (int index : bitVector.set_bits()) { ... }

instead of:

for (int index = bitVector.find_first(); index != -1;
       index = bitVector.find_next(index)) { ... }

gottesmm · 2020-06-16T19:01:09Z

@swift-ci test

gottesmm · 2020-06-16T19:02:56Z

lib/SILOptimizer/Utils/Generics.cpp

@@ -1969,6 +1997,10 @@ static ApplySite replaceWithSpecializedCallee(ApplySite applySite,
    assert(resultBlock->getSinglePredecessorBlock() == tai->getParent());
    auto *newTAI = builder.createTryApply(loc, callee, subs, arguments,
                                          resultBlock, tai->getErrorBB());
+    FullApplySite(newTAI).insertAfterFullEvaluation(
+        [&](SILBasicBlock::iterator insertPt) {
+          cleanupCallArguments(builder, loc, arguments, argsNeedEndBorrow);


Hmmm... I think I need to create a new SILBuilder here.

I am going to try to break this. This to not fail says we are missing code coverage.

swift-ci · 2020-06-16T19:03:59Z

Build failed
Swift Test OS X Platform
Git Sha - 745f482a722942492f37429ef61b2ff1be26468b

swift-ci · 2020-06-16T19:04:34Z

Build failed
Swift Test Linux Platform
Git Sha - 745f482a722942492f37429ef61b2ff1be26468b

eeckstein

lgtm!

gottesmm · 2020-06-21T05:59:00Z

@swift-ci test

swift-ci · 2020-06-21T06:22:45Z

Build failed
Swift Test OS X Platform
Git Sha - 2e6f46c0627e4f51518f83cf06ed92856f4c1ad3

swift-ci · 2020-06-21T07:15:30Z

Build failed
Swift Test Linux Platform
Git Sha - 2e6f46c0627e4f51518f83cf06ed92856f4c1ad3

gottesmm · 2020-06-23T01:09:08Z

I was correct that we were missing code coverage. I added the missing code coverage of try_apply/begin_apply.

gottesmm · 2020-06-23T01:09:14Z

@swift-ci test

gottesmm · 2020-06-23T01:09:18Z

@swift-ci test

gottesmm · 2020-06-23T01:09:23Z

@swift-ci test

swift-ci · 2020-06-23T01:11:39Z

Build failed
Swift Test OS X Platform
Git Sha - 2e6f46c0627e4f51518f83cf06ed92856f4c1ad3

swift-ci · 2020-06-23T01:12:09Z

Build failed
Swift Test Linux Platform
Git Sha - 2e6f46c0627e4f51518f83cf06ed92856f4c1ad3

swift-ci · 2020-06-23T01:13:56Z

Build failed
Swift Test OS X Platform
Git Sha - 2e6f46c0627e4f51518f83cf06ed92856f4c1ad3

gottesmm · 2020-06-23T01:18:18Z

@swift-ci test windows platform

gottesmm · 2020-06-23T01:34:53Z

@swift-ci test OS X platform

gottesmm · 2020-06-23T01:35:00Z

OS X failure was a sandbox issue

swift-ci · 2020-06-23T02:20:59Z

Build failed
Swift Test OS X Platform
Git Sha - 77b094a470e872ee0814eb992487eded08dd6e40

gottesmm · 2020-06-23T22:05:48Z

I took a look at why we are failing/hitting the verifier error. The reason why is that there is a debug_value on self that is after the store [init] of self at the beginning of a function to materialize the argument.

gottesmm · 2020-06-29T21:09:43Z

Reproduced the error.

gottesmm · 2020-06-29T21:33:29Z

Interesting:

// thunk for @escaping @callee_guaranteed (@inout A, @guaranteed PathArgument) -> (@error @owned Error)
sil shared [transparent] [serializable] [reabstraction_thunk] [ossa] @$sx10TSCUtility12PathArgumentVs5Error_pIeglgzo_xACsAD_pIeglnzo_8Commands11ToolOptionsCRbzlTR : $@convention(thin) <Options where Options : ToolOptions> (@inout Options, @in_guaranteed PathArgument, @guaranteed @callee_guaranteed (@inout Options, @guaranteed PathArgument) -> @error Error) -> @error Error {
// %0                                             // user: %4
// %1                                             // user: %3
// %2                                             // user: %4
bb0(%0 : $*Options, %1 : $*PathArgument, %2 : @guaranteed $@callee_guaranteed (@inout Options, @guaranteed PathArgument) -> @error Error):
  %3 = load_borrow %1 : $*PathArgument            // users: %7, %10, %4
  try_apply %2(%0, %3) : $@callee_guaranteed (@inout Options, @guaranteed PathArgument) -> @error Error, normal bb1, error bb2 // id: %4

bb1(%5 : $()):                                    // Preds: bb0
  %6 = tuple ()                                   // user: %8
  end_borrow %3 : $PathArgument                   // id: %7
  return %6 : $()                                 // id: %8

// %9                                             // user: %11
bb2(%9 : @owned $Error):                          // Preds: bb0
  end_borrow %3 : $PathArgument                   // id: %10
  throw %9 : $Error                               // id: %11
} // end sil function '$sx10TSCUtility12PathArgumentVs5Error_pIeglgzo_xACsAD_pIeglnzo_8Commands11ToolOptionsCRbzlTR'

gottesmm · 2020-06-30T00:00:42Z

Bug in the reabstract thunk generation code.

gottesmm · 2020-06-30T00:05:24Z

Found the bug. It was my bug. Thank god for ownership verification = ).

gottesmm · 2020-07-01T03:03:51Z

I am going to try again tonight.

gottesmm · 2020-07-01T06:33:22Z

Tracked down the problem. The problem was that there was some misleading code that seemed like it was only handling apply inst. Instead it was also handling try_apply, but returning the normal block argument as the "result". This seems like a general useful property, so I put it into a helper on ApplySite. Now it all works.

gottesmm · 2020-07-01T06:33:29Z

@swift-ci test

gottesmm · 2020-07-01T06:33:34Z

@swift-ci test

gottesmm · 2020-07-01T06:33:38Z

@swift-ci test

gottesmm · 2020-07-01T06:33:50Z

@swift-ci test source compatibility

gottesmm · 2020-07-01T06:33:53Z

@swift-ci test source compatibility

gottesmm · 2020-07-01T06:33:56Z

@swift-ci test source compatibility

swift-ci · 2020-07-01T06:36:38Z

Build failed
Swift Test Linux Platform
Git Sha - f4d40c6d974b4288ee3b61e01c9ccf629a12bc67

swift-ci · 2020-07-01T06:37:16Z

Build failed
Swift Test OS X Platform
Git Sha - f4d40c6d974b4288ee3b61e01c9ccf629a12bc67

gottesmm · 2020-07-01T07:34:21Z

@swift-ci test windows platform

gottesmm · 2020-07-02T03:39:38Z

Found more missing test coverage. We don't ever generate one of these reabstraction thunks with @out parameters in the test suite...

…eric specialization tests behind a flag. The idea is that this will let me remove these assertions that were in place to make sure we were really conservative around specializing ownership code. For me to remove that I need to be able to actually test out this code (since I think there are some code paths where this will trigger in other parts of the compiler now). So to work out the kinks, I added a flag that allows for the generic specializer to process ownership code and translated most of the .sil test cases/fixed any bugs that I found. This hopefully will expose anything that is missing. NOTE: I have not enabled the generic specializer running in ownership in the pipeline. This is just a step in that direction by adding tests/etc.

gottesmm · 2020-07-03T09:55:01Z

@swift-ci test

gottesmm · 2020-07-03T09:55:08Z

@swift-ci test

gottesmm · 2020-07-03T09:55:15Z

@swift-ci test

gottesmm · 2020-07-03T09:55:24Z

@swift-ci test source compatibility

gottesmm · 2020-07-03T09:55:29Z

@swift-ci test source compatibility

gottesmm · 2020-07-03T09:55:34Z

@swift-ci test source compatibility

gottesmm · 2020-07-03T09:55:42Z

@swift-ci test windows platform

gottesmm · 2020-07-03T12:32:33Z

@swift-ci test windows platform

gottesmm · 2020-07-04T19:41:05Z

Looks like Source Compatibility matches the state on ci.swift.org. It isn't this commit.

gottesmm requested a review from eeckstein June 15, 2020 23:06

gottesmm commented Jun 15, 2020

View reviewed changes

eeckstein requested changes Jun 16, 2020

View reviewed changes

gottesmm force-pushed the pr-92e92ba00c4c145dbd650430947c43d7b3a743d5 branch from 745f482 to 2e6f46c Compare June 16, 2020 19:00

gottesmm requested a review from eeckstein June 16, 2020 19:00

gottesmm commented Jun 16, 2020

View reviewed changes

eeckstein approved these changes Jun 16, 2020

View reviewed changes

gottesmm force-pushed the pr-92e92ba00c4c145dbd650430947c43d7b3a743d5 branch 2 times, most recently from b3e1043 to 77b094a Compare June 23, 2020 01:08

gottesmm force-pushed the pr-92e92ba00c4c145dbd650430947c43d7b3a743d5 branch from 77b094a to f4d40c6 Compare June 26, 2020 20:23

gottesmm force-pushed the pr-92e92ba00c4c145dbd650430947c43d7b3a743d5 branch from f4d40c6 to 275a1e6 Compare July 1, 2020 06:32

gottesmm force-pushed the pr-92e92ba00c4c145dbd650430947c43d7b3a743d5 branch from 275a1e6 to 1b97c03 Compare July 3, 2020 09:54

gottesmm merged commit 8a1f2c6 into swiftlang:master Jul 4, 2020

gottesmm deleted the pr-92e92ba00c4c145dbd650430947c43d7b3a743d5 branch July 4, 2020 19:41

[ownership] Loosen restrictions around what we specialize and add generic specialization tests behind a flag. #32397

[ownership] Loosen restrictions around what we specialize and add generic specialization tests behind a flag. #32397

Uh oh!

Conversation

gottesmm commented Jun 15, 2020

Uh oh!

gottesmm Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

eeckstein Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

gottesmm Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

eeckstein left a comment

Choose a reason for hiding this comment

Uh oh!

eeckstein Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

gottesmm commented Jun 16, 2020

Uh oh!

gottesmm Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

gottesmm Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Jun 16, 2020

Uh oh!

swift-ci commented Jun 16, 2020

Uh oh!

eeckstein left a comment

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Jun 21, 2020

Uh oh!

swift-ci commented Jun 21, 2020

Uh oh!

swift-ci commented Jun 21, 2020

Uh oh!

gottesmm commented Jun 23, 2020

Uh oh!

gottesmm commented Jun 23, 2020

Uh oh!

gottesmm commented Jun 23, 2020

Uh oh!

gottesmm commented Jun 23, 2020

Uh oh!

swift-ci commented Jun 23, 2020

Uh oh!

swift-ci commented Jun 23, 2020

Uh oh!

swift-ci commented Jun 23, 2020

Uh oh!

gottesmm commented Jun 23, 2020

Uh oh!

gottesmm commented Jun 23, 2020

Uh oh!

gottesmm commented Jun 23, 2020

Uh oh!

swift-ci commented Jun 23, 2020

Uh oh!

gottesmm commented Jun 23, 2020

Uh oh!

gottesmm commented Jun 29, 2020

Uh oh!

gottesmm commented Jun 29, 2020

Uh oh!

gottesmm commented Jun 30, 2020

Uh oh!

gottesmm commented Jun 30, 2020

Uh oh!

gottesmm commented Jul 1, 2020

Uh oh!

gottesmm commented Jul 1, 2020

gottesmm commented Jun 16, 2020 •

edited

Loading