Implement generalized accessors using yield-once coroutines

[rjmccall]

For now, the accessors have been underscored as _read and _modify. I'll prepare an evolution proposal for this feature which should allow us to remove the underscores or, y'know, rename them to purple and lettuce.

_read accessors do not make any effort yet to avoid copying the value being yielded. I'll work on it in follow-up patches.

Opaque accesses to properties and subscripts defined with _modify accessors will use an inefficient materializeForSet pattern that materializes the value to a temporary instead of accessing it in-place. That will be fixed by migrating to modify over materializeForSet, which is next up after the read optimizations.

SIL ownership verification doesn't pass yet for the test cases here because of a general fault in SILGen where borrows can outlive their borrowed value due to being cleaned up on the general cleanup stack when the borrowed value is cleaned up on the formal-access stack. Michael, Andy, and I discussed various ways to fix this, but it seems clear to me that it's not in any way specific to coroutine accesses.

rdar://35399664

[rjmccall]

@swift-ci Please test.

[rjmccall]

@atrick I'd appreciate a review of the SILOptimizer changes here.
@slavapestov @jckarter If y'all wouldn't mind looking over the Sema and SILGen changes.

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 92044016cdc37b43a86746df19bcf95e4deb50a1

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 92044016cdc37b43a86746df19bcf95e4deb50a1

[rjmccall]

@swift-ci Please test.

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 92044016cdc37b43a86746df19bcf95e4deb50a1

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 92044016cdc37b43a86746df19bcf95e4deb50a1

[rjmccall]

@swift-ci Please test.

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 217c651ee7fd63e31eb46be855a8d970715c6ab2

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 217c651ee7fd63e31eb46be855a8d970715c6ab2

[rintaro]

I'm not a fan of mutating token in isStartOfStmt(). For example, skipUntil family functions call this method that may accidentally mutates it. I think it's better to leave it tok::identifier here, then setKind() in parseStmt(). In case you really need this, please add case tok::kw_yield: return true in the switch at least.

[rjmccall]

Okay. How's this look? #18179

[atrick]

@rjmccall it looks to me like SIL formal access on the begin_apply's address will be handled correctly with this patch.

I suspect that side effect analysis will just be conservative at the begin_apply, in the sense that it continues analyzing the function past the yield, summarizing both the initialization and resume effects at the begin_apply.

The end_apply will be a problem if there's no restriction on what users can do in the coroutine's "tail". As such, I'm a little surprised that it isn't also modeled as an ApplySite. In the current implementation, its side effects are conservative, and the effects on the call graph should already be modeled by the begin_apply, so it may be ok in general. But I'm pretty sure AccessedStorageAnalysis will be wrong. It currently assumes that formal access can only occur at an ApplySite or begin_access. Ultimately, that should be fixed to view the end_apply's effects as if it were an ApplySite. The most natural way to do that would be to teach SideEffectAnalysis in general to treat end_apply as an ApplySite. How do you feel about that? I don't see an alternative short of preventing the user from performing formal access in the coroutine tail. Of course, for now, I could just hack SideEffectAnalysis to call setWorstEffects at an end_apply.

[rjmccall]

There are no restrictions about what can happen in the end_apply; that instruction runs the coroutine to completion. Note that borrows and inout accesses that are initiated for arguments to begin_apply must last until the end of the end_apply / abort_apply, so you may need some special treatment.

Beyond that, I don't know what significance treating end_apply as an apply site has beyond having pessimal side effects.

[atrick]

Two things.

1. Correctness. I want to rely on the fact that only a begin_access or ApplySite may initiate formal access. I need to extend that to include end_apply now, and maybe abort_apply† Note that there's no other non-apply instruction quite like this that can execute arbitrary SIL operations (aside from deinit). So, more generally, if I create a new opcode and declare that something can only happen via that opcode, now I need to check for: the opcode itself, all ApplySites, all end_apply's.

2. Strength of the analysis. In the long run, we don't just want to assume an end_apply could access anything, unless we also assume it will be inlined whenever possible.

†I'd like to assume that an arbitrary release, which may call deinit, does not need to check at runtime that a global/property accessed by the deinit may conflict with something being accessed in the caller. That will be prohibitive for access marker merging/elimination. That's shady though because it's not quite "as if" the deinit ran later.

[rjmccall]

Note that abort_apply can run arbitrary cleanups including, yes, releases, but also defer blocks. So yeah, anything can happen there. If you think it makes sense to treat it and end_apply as ApplySites, okay, but I suspect that that isn't right for literally any other client of ApplySite.

[atrick]

I think it's conceptually right to make end_apply and apply everywhere. But I don't think it's worth it. There are too many places in the code that assume an ApplySite is either Full or Partial. This would be neither. It would have a callee but no normal arguments. I think that's sort of what you're saying.

In practice, giving end_apply arbitrary side effects will end up being conservatively correct in almost all cases. AccessedStorageAnalysis can be fixed easily. SIL developers will just need to remember that there are other apply-like things to check for now.