[SIL] Only visit final partial_apply [on_stack] lifetime ends.

[nate-chandler]

In OSSA, the partial_apply [on_stack] instruction produces a value with odd characteristics that correspond to the fact that it is lowered to a stack-allocating instruction. Among these characteristics is the fact that copies of such values aren't load bearing.

When visiting the lifetime-ending uses of a partial_apply [on_stack] the lifetime ending uses of (transitive) copies of the partial_apply must be considered as well. Otherwise, the borrow scope it defines may be incorrectly short:

%closure = partial_apply %fn(%value) // borrows %value
%closure2 = copy_value %closure
destroy_value %closure // does _not_ end borrow of %value!
...
destroy_value %closure2 // ends borrow of %value
...
destroy_value %value

Furthermore, only the final such destroys actually count as the real lifetime ends. At least one client (OME) relies on visitOnStackLifetimeEnds visiting at most a single lifetime end on any path.

Rewrite the utility to use PrunedLiveness, tracking only destroys of copies and forwards. The final destroys are the destroys on the boundary.

Also, add here a couple other changes:

• Made it a bit easier to debug CopyPropagation, where the original problem showed up, by adding subpass bailouts.
• Did the same for DestroyAddrHoisting, where a different issue (a missing !) introduced by a previous version of this patch appeared to show up.
• Invalidated instructions in TempRValueOpt when lifetime completion had an effect. This lack of invalidation resulted in not verifying or printing the SIL after the pass with sil-print-function despite the fact that it had indeed been changed (without using more exotic flags like sil-verify-force-analysis), obscuring the actual source of the issue introduced by a previous version of this patch.

rdar://142636711

[nate-chandler]

@swift-ci please test

[nate-chandler]

@swift-ci please apple silicon benchmark

[nate-chandler]

@swift-ci please test source compatibility

[nate-chandler]

Source compat failures match failures in baseline:

Failures:
  FAIL: vapor_jwt-kit, 5.10, faa6f7, Swift Package
  FAIL: vapor_jwt, 5.10, f240c5, Swift Package

[eeckstein]

lifetime ending uses of (transitive) copies of the partial_apply must be considered as well.

I'm wondering if this is the right approach. I'm pretty sure that other utilities, like OwnershipLiveness.swift doesn't handle this correctly, too.
It would be interesting to see where a copy of a non-escaping closure comes from. A non-escaping closure can only be passed to a function and called.
If we can avoid a non-escaping closure to be copied, we can avoid this problem at all.

[eeckstein]

The problem I have with this is that it's very complicated to compute this implicit borrow scope. It's already bad enough that and partial_apply's argument borrow-scope is defined by a forward-extended liverange. But also needing to visit copies (and "parallel" liveranges) transitively is terrible.

[nate-chandler]

If we can avoid a non-escaping closure to be copied

Yeah, that would definitely be nice. Something that's been observed in the past when similar problems have arisen is that such values ought to be represented as non-copyable values. Unfortunately, that's not the current state of affairs. And also unfortunately, SILGen and passes both introduce copies in numerous places.

[eeckstein]

Suggestion:

• add a utility which cleans up copies of non-escaping closures - which is basically copy-propagation
• run this utility in silgencleanup
• add a check in the SILVerifier that non-escaping closures are not copied.

We can detect optimizations which introduce such copies easily and run the utility at the end of such passes.
I think this is the cleaner solution.
And also the easier one because otherwise we would have to add support of copy_value in all our utilities. Not updating other utilities would be a time bomb.

[nate-chandler]

Pulled out the miscellaneous improvements into #78682 while we discuss the main fix here.

[nate-chandler]

Unfortunately, it looks like more interesting changes representational would be required to eliminate copies. Here is some simplified SIL from the _Backtracing library:

  %closure = partial_apply [callee_guaranteed] [on_stack] %f1
  %copy = copy_value %closure
  // function_ref thunk for @callee_guaranteed (@unowned _CSTypeRef, @unowned _CSTypeRef) -> ()
  %thunk = function_ref @$sSo10_CSTypeRefVABIgyy_A2BIegyy_TR
  // NOT on_stack -- forwarding consume
  %thunk_closure = partial_apply [callee_guaranteed] %thunk(%copy)
  %thunk_copy = copy_value %thunk_closure
  %block_storage = alloc_stack $@block_storage
  %thunk_addr = project_block_storage %block_storage
  store %thunk_copy to [init] %thunk_addr

[atrick]

The fundamental idea behind OSSA is that we do not run liveness analysis to find a value's ownership lifetime.
Originally, we wanted partial_apply [on_stack] to produce a borrowed value. But since then, we've added all the support necessary to handle it as an owned value. This works reasonably well because, in general, an owned value can depend on some borrow scope. We need to support that for mark_dependence anyway. The alternative would be to make partial_apply [on_stack] and mark_dependence scoped operations, where we maintain an end_dependence on all paths. This would be pretty nasty.

Now that we have formal abstractions for ForwardingInstruction and OwnershipTransitionInstruction, it should be robust and efficient to find the end points of both partial_apply [on_stack] and mark_dependence.

As pointed out here, a copy of a dependent value does normally not carry that dependency. So, it does not make sense for partial_apply [on_stack] to be copied. That should be fixed up when we do the on-stack conversion.

The project_block_storage SIL patterns are an even worse ownership atrocity. It's probably worth fixing that anyway, e.g. forcing it to be a store_borrow or just creating a new SIL instruction to replace the whole pattern.

Incidentally, partial_apply [on_stack] should have its own opcode since it is structurally different from a normal partial_apply.

[eeckstein]

I think the cleanest solution would be to add begin/end_borrow scopes for the partial_apply's arguments.

[atrick]

I don't see how adding borrow scopes solves the problem. The problem just moves around to different places. Ultimately, we need to figure out how to represent the fact that each owned copy of the closure depends on the captures. So either we make the copies illegal or add mark_dependence to all of them so they all depend on the original partial_apply.

[eeckstein]

Well, it solves the problem that the lifetime of partial_apply arguments is defined explicitly and not implicitly by complicated traversing of forwarding instructions (and maybe copies) - like we do for all other borrow scopes, too. The ownership verifier still has to check that the forward-extended lifetime of the closure does not extend the borrow scopes of the arguments.

make the copies illegal

That's what we should do anyway.

[nate-chandler]

@swift-ci please test

[nate-chandler]

@swift-ci please test source compatibility

[nate-chandler]

@swift-ci please apple silicon benchmark

[nate-chandler]

Merging as-is to unblock OSSA progress. Filed rdar://143821866 (Fix representation of partial_apply [on_stack] to ban copies) to track the correct solution.