[SILOptimizer] Generalize optimization of static keypaths

[NobodyNada]

We have an optimization in SILCombiner (introduced in #24929) that inlines compile-time constant key paths by performing the property access directly instead of calling a runtime function (leading to huge performance gains e.g. for heavy use of @dynamicMemberLookup). However, this optimization previously only supported key paths which solely access stored properties, so computed properties, optional chaining, etc. still had to call a runtime function. This commit generalizes the optimization to support all types of key paths.

This is my first time contributing to any sort of compiler development, so it's likely I've overlooked something. The previous iteration of this optimzation essentially computed the stored property offsets and returned the resulting address. I still used with a design that projects the key path to an address since that's what's returned by the runtime functions that we're replacing, but my implementation is more complex because projections for most types of key path components need both setup and teardown logic to e.g. deallocate temporaries, call getters and setters, or re-wrap optionals.

I created an abstract class called KeyPathProjector. It has a method called project, which accepts an access type (get, set, or modify) and a callback lambda. This method inserts SIL instructions to project the key path to an address, invokes the callback, and then inserts instructions to tear down the projection. A complete key path is handled (internally to KeyPathProjector.cpp) by creating a "chained" projector: each projector in the chain is responsible for projecting a single component on top of the previously projected result. For each type of key path component, there is one subclass of KeyPathProjector (again, all of these are implementation details of KeyPathProjector.cpp).

There are a few special cases (such as access enforcement and optional chaining), but I think my logic works, and it passes all the tests I've tried. I do have a couple concerns though:

• Subscripts in key paths are treated as computed properties with indices, and they require an additional context pointer to be passed to the property accessor. The key path projector needs to create this context object with the correct memory layout; I implemented this by creating a tuple which contains the context operands. Is a tuple guaranteed to have the correct memory layout? Will this work in all cases, and is it guaranteed to work in the future?
• Some code in IRGen, along with the key path ABI documentation, seemed to make references to including a "generic environment" along with these context pointers. However, SILVerifier doesn't seem to know about this, and I haven't been able to figure out anything more. Is this something I need to account for? Under what circumstances would I need to pass a generic environment to an accessor, and what exactly would that involve?

[eeckstein]

Nice!

Tow general comments:

1. You are using a class hierarchy to implement the different kind of projections. That's nice. In addition you are using closures for the callback. Given that you already have the class hierarchy, you don't need the closures, which would make the code a bit easier to understand: If you store child-links in the projectors, you could do something like:

void project(AccessType accessType, SILValue parentValue) {
  // Stuff needed before the child projection, e.g. compute myAddress
  child->project(accessType, myAddress)
  // Stuff needed after the child projection
}

2. About the tests: For each projector type, can you please add tests which use the non-trivial types GenClass or SimpleClass as payloads. This will test if the ownership is correct in the generated SIL, i.e. no over-release and no memory leaks.
   Also: can you please add tests which test the combination of several projectors, i.e. key paths with multiple different components?

[eeckstein]

@jckarter Can you please answer the questions about the subscript context and the generic environment?

[NobodyNada]

@eeckstein Thanks for the feedback!

Regarding your first point: I used the closures in addition to the class hierarchy because the access type needs to flow backwards from leaf to root. (For example, when projecting someClass.someStruct.someProperty = 1, someProperty is set, someStruct is modified, and someClass is read.) The closure-based approach allows the child projectors to decide what access type the parent should be invoked with.

The simplest alternative I can think of is to instead store the access types as member variables, determining them ahead-of-time when creating the projector in KeyPathProjector::create. Shall I go ahead with that approach?

[eeckstein]

@NobodyNada Ah, I see. Makes sense (no need to change it).

[jckarter]

Rather than try to reconstruct the memory layout of the generic argument buffer, I think it'd be better to implement a @convention for key path accessors that IRGen lowers as passing the generic arguments and indexes indirectly in a buffer with the right order. That would allow IRGen to remain the point of truth for how the generic arguments get laid out, as well as get rid of some clumsy code in GenKeyPath where it has to rethunk accessor functions because the convention can't be represented directly in SIL. Doing this would also allow high-level SIL optimizations to be more effective after the key path application is reduced, since the resulting apply instructions would retain the high-level argument information, so they should be easier to inline or apply other interprocedural optimizations to.

To keep this PR tractable, maybe you make it so that this patch only supports nongeneric computed components for now, and try implementing a key_path_accessor convention and support for generic components in a followup patch.

Thanks for implementing this!

[zoecarver]

It might make sense to make this its own pass. One less thing in SILCombine's scope and it probably doesn't benefit from being run multiple times.

[NobodyNada]

@jckarter Thanks! I’ll go with what you suggested and leave indices/generic environments for a followup PR. I don’t think I’ll have time to work on that followup PR anytime within the next few months, but I may have a chance around summertime. I do have one question though: Under what circumstances does a generic environment get passed to a subscript accessor? I experimented a bit but didn’t come up with a test case that triggered that.

@zoecarver Sounds good, I can make that change. At what point in the pipeline should the pass run? My best guess is the beginning of addHighLevelLoopOptPasses, but I don’t understand the optimizer well enough to be sure.

[jckarter]

@NobodyNada Sounds good, no rush! A generic environment ought to get passed down when a key path literal appears in an unspecialized generic context, and its index types are dependent on that generic context. For example:

struct Blah {
  subscript<T>(x: T) -> T { return x }
}

func foo<T: Hashable>(x: T) {
  _ = \Blah.[x]
}

[NobodyNada]

@eeckstein @jckarter Thanks for the feedback! I finally got around to making those changes; sorry for the delay.

[jckarter]

@swift-ci Please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - e1ceb4f

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - e1ceb4f

[NobodyNada]

Hmm, both of the failing tests pass on my machine. It looks like the tests only failed on Linux (it says the Mac tests were aborted), so I'll try to investigate on a Linux machine sometime this week. It seems strange that those crashes would be platform-specific though.

[jckarter]

@swift-ci Please test OS X

[theblixguy]

It seems like GettablePropertyProjector::project is tripping a few asserts:

1. swift::Lowering::AbstractionPattern::initSwiftType(swift::CanGenericSignature, swift::CanType, swift::Lowering::AbstractionPattern::Kind): Assertion `signature || !origType->hasTypeParameter()' failed.

2. virtual void GettablePropertyProjector::project(swift::KeyPathProjector::AccessType, std::function<void (SILValue)>): Assertion `accessType == AccessType::Get && "property is not settable"' failed.

Let's see if those tests crash on macOS as well.

[NobodyNada]

Hmm, I could not reproduce the failing tests on Ubuntu 18.04. I can't access the CI build logs anymore to compare; have they been archived somewhere or would we have to re-run the tests to get new logs?

[eeckstein]

@swift-ci test linux

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 48132ea

[jckarter]

@swift-ci Please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 48132ea

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 48132ea

[NobodyNada]

I believe I fixed all test failures. There were three issues:

• When a computed property returns a generic, the accessor's function
  type may involve a type parameter that needs to be resolved using
  the key path instruction's substitution map. I did not properly handle this case, causing the stdlb/KeyPath.swift test to fail. Fixed in d7acf12.
• When setting a key path, different projectors inconsistently provided initialized or uninitialized memory to the callback. This caused another crash in stdlib/KeyPath.swift; I fixed it in 1feead8 by always providing an uninitialized address.
• SILGen's emitKeyPathComponentForDecl did not properly handle @testable imports, since it only checked if the setter was accessible from the current module rather than the current function. This caused a failure in Interpreter/testable_key_path.swift, since the generated SIL tried to set a property which SILGen incorrectly assumed was get-only. As far as I understand it, this was a preexisting issue in SILGen that was surfaced by my key path projector code. Fixed in a06fe96

The failures were not platform-specific after all, but they only showed up on Linux CI because the Mac CI bot ran the tests with optimizations disabled.

[theblixguy]

@swift-ci please test

[jckarter]

Thanks @NobodyNada!

[jckarter]

Unfortunately, it looks like this still leads to a test failure in stdlib/KeyPath.swift on 32-bit slices (which aren't covered by our normal PR testing):

[ RUN      ] key paths.optional force-unwrapping
stderr>>> Assertion failed: (getStrongExtraRefCount() + 1 >= dec && "releasing reference whose refcount is already zero"), function decrementStrongExtraRefCount, file /Users/buildslave/jenkins/workspace/fs-swift-unreleased-master-tools_RA-stdlib_RA/swift/include/swift/Runtime/../../../stdlib/public/SwiftShims/RefCount.h, line 576.
stderr>>> CRASHED: SIGABRT
the test crashed unexpectedly

You should be able to use build-script with the -i flag to configure the build to include iOS and Simulator slices, and try the test-iphonesimulator-i386/stdlib/KeyPath.swift test as an example 32-bit slice. I'll revert this for the time being to avoid the regression.

[NobodyNada]

@jckarter I haven’t run the tests locally, but line 434 of KeyPathProjector.swift should be removed since optAddr is uninitialized here (I must have missed that in 1feead8). Shall I open another PR?

[jckarter]

@NobodyNada Sure, thanks for the quick response!

[jckarter]

@NobodyNada I went ahead and reverted this in #29946. Let me know when you have an updated patch so we can get this back in. Thanks again!

[NobodyNada]

@jckarter I've opened #30003 and confirmed it fixes the crash. Sorry for the delay, it took me a few clean rebuilds before I had the right configuration to reproduce the issue.