Centralize KeyPath accessor calling convention logic to IRGen

[kateinoigakukun]

Motivation

Resolves rdar://113910207.

KeyPath's getter/setter/hash/equals functions have their own calling convention, which receives generic arguments and embedded indices from a given KeyPath argument buffer.
The convention was previously implemented by:

1. Accepting an argument buffer as an UnsafeRawPointer and casting it to indices tuple pointer in SIL.

struct S {
  subscript<T>(x: Int) -> Int { get }
}

// keypath getter accessor for `\S.[x]`
sil hidden @testGetter : $@convention(thin) (@in_guaranteed S, UnsafeRawPointer) -> @out Int {
bb0(%0 : $*Int, %1 : $*S, %2 : $UnsafeRawPointer):
  %3 = load %1 : $*S
  %4 = pointer_to_address %2 : $UnsafeRawPointer to $*(Int)
}

2. Bind generic arguments info from the given argument buffer while emitting prologue in IRGen by creating a new forwarding thunk.

; Accessor generated at SILGen
define swiftcc void @testGetter(%TSi* noalias nocapture sret(%TSi) %0, %T3bar4YeahV* noalias nocapture %1, i8* %2) {
  ...
}
; Thunk generated at IRGen
define swiftcc void @keypath_get(%TSi* noalias nocapture sret(%TSi) %0, %T3bar4YeahV* noalias nocapture %1, i8* %2, i64 %3) {
  ...
  ; forward arguments (also unpack witness tables from the buffer if needed)
  call swiftcc void @testGetter(%0, %1, %2)
}

This 2-phase lowering approach was not ideal, as it blocked KeyPath projection optimization ¹, and also required having a target arch specific signature lowering logic in SIL-level ².

Description

This patch centralizes the KeyPath accessor calling convention logic to IRGen, by introducing @convention(keypath_accessor_XXX) convention in SIL and lowering it in IRGen. The accessor functions now accept indices as tuple instead of UnsafeRawPointer in SIL level.

// keypath getter accessor for `\S.[x]`
sil hidden @testGetter : $@convention(thin) (@in_guaranteed S, @in_guaranteed (Int)) -> @out Int {
bb0(%0 : $*Int, %1 : $*S, %2 : $*(Int)):
   ...
}

This change unblocks the KeyPath projection optimization while capturing subscript indices, and also makes it easier to support WebAssembly target.

ABI concerns

• The layout of the KeyPath argument buffer itself is not changed
• Thunk functions generation is now skipped except for referencing external property descriptors to make relative pointers, but the thunk function symbols are hidden, so doesn't break ABI
• Accessor functions now always have argument buffer pointer and its size integer in parameter list even the keypath doesn't have indices or generic parameter captures. But caller sites unconditionally pass them and works well, so the new form should be more strict and has no effect.

Merge plan

TBD

Footnotes

1. https://github.com/apple/swift/pull/28799 ↩

2. https://forums.swift.org/t/wasm-support/16087/21 ↩

[kateinoigakukun]

I don't think it's the ideal design to have conventions for each signature, but I couldn't come up with better ideas...

Currently, accessors are represented by the following in SIL:

sil hidden @testGetter : $@convention(keypath_accessor_getter) (@in_guaranteed S, @in_guaranteed (S, C)) -> @out Int
sil hidden @testSetter : $@convention(keypath_accessor_setter) (@in_guaranteed Int, @in_guaranteed S, @in_guaranteed (S, C)) -> ()
sil hidden @testEq     : $@convention(keypath_accessor_equals) (@in_guaranteed (S, C), @in_guaranteed (S, C)) -> Bool
sil hidden @testHash   : $@convention(keypath_accessor_hash)   (@in_guaranteed (S, C)) -> Int

As a possible idea, if having a single @convention(keypath_accessor) and put new ParameterConvention attributes like @keypath_argument_buffer:

sil hidden @testGetter : $@convention(keypath_accessor) (@in_guaranteed S, @keypath_argument_buffer (S, C)) -> @out Int
sil hidden @testSetter : $@convention(keypath_accessor) (@in_guaranteed Int, @in_guaranteed S, @keypath_argument_buffer (S, C)) -> ()
sil hidden @testEq     : $@convention(keypath_accessor) (@keypath_argument_buffer (S, C), @keypath_argument_buffer (S, C)) -> Bool
sil hidden @testHash   : $@convention(keypath_accessor) (@keypath_argument_buffer (S, C)) -> Int

But unlike other ParameterConventions, @keypath_argument_buffer is not only how the parameter should be passed but also a marker to determine where generic parameters should be retrieved from. So I'm unsure whether this new @keypath_argument_buffer matches the ParameterConvention's concept or not.

What do you think about this, and do you have any other ideas? It would be much appreciated if I can have any comment from @jckarter 🙏

[jckarter]

I think it's better to have one convention if possible. The argument buffer from which we bind generic parameters is always the last argument, so we could have that be an implicit property of the convention, just like how the self or context argument for @convention(method) is always the final argument.

[kateinoigakukun]

Thank you for your comment 🙏
Oh, I overlooked @convention(method)! It makes sense to me to have such implicit property. I'll update the patch.

[jckarter]

Looking a bit longer term, for partial apply elimination, I want to extend SILBoxType to have a "nonescaping" flavor to explicitl represent the context parameter for nonescaping closures. These have the same need to capture and rebind the generic environment from a parameter (as represented by the @capturesGenericEnvironment attribute), so maybe once that is implemented we could also use it for keypath accessors with the normal thin convention. That's a ways away, though, so I think introducing a new convention is fine for now.

[kateinoigakukun]

@jckarter While updating the patch to have one unified convention, I found we can't have an implicit property that takes generic parameters from the last argument because getter and setter accessors don't always have indices.

IRGen needs to know whether a keypath getter/setter accessor function has indices or not because:

• IRGen needs to add dummy keypath argument buffer pointer to IR-level parameters to have a stable signature
• If the accessor doesn't have indices but has generic parameters, it can omit a GEP to offset the argument buffer pointer to retrieve generic parameters at runtime.

SILGen of course knows hasIndices info but IRGen doesn't if we use one unified convention and no explicit marker.

For example, Getter with indices ((base: Foo, indices: (Int, String))) and setter without indices ((value: Bar, base: (Int, String))) have the same number of parameters and both can have trailing tuple parameter.
The former doesn't need dummy keypath argument buffer pointer to IR-level parameters, but the latter needs it. However, they cannot be distinguished during IRGen as far as I know.

I also tried to put a trailing empty tuple parameter even though the accessor doesn't have indices during SILGen, but we still need hasIndices info at IRGen to avoid extra GEP.

So I think we need something marker to propagate hasIndices info. I'd like to hear your thoughts and which direction we should go (four conventions or parameter attribute or other) 🙏

[kateinoigakukun]

@jckarter gentle ping :)

[tbkka]

CC: @atrick @meg-gupta @nate-chandler

[aschwaighofer]

I think we need something marker to propagate hasIndices info. I'd like to hear your thoughts and which direction we should go (four conventions or parameter attribute or other)

I think you can have an attribute on the function rather than parameter attribute. The reason why I would not want a new parameter attribute is that the SIL optimizer would have to understand its semantics. The function attribute it can ignore. (edit: see below)

[aschwaighofer]

Thinking about this more, using a function attribute would be kinda unfortunate -- the signature of a function should be all that is needed to determine its ABI. Using a function attribute would require to look at the function declaration to get the final ABI that would be very unfortunate. Ignore my above suggestion.

[kateinoigakukun]

@swift-ci Please smoke test

[kateinoigakukun]

@swift-ci Please smoke test

[MaxDesiatov]

@swift-ci build toolchain

[MaxDesiatov]

preset=buildbot_incremental_linux_crosscompile_wasm
@swift-ci Please test with preset Linux Platform

[kateinoigakukun]

@swift-ci Please smoke test Windows platform

[kateinoigakukun]

@swift-ci build toolchain Windows platform

[kateinoigakukun]

The preset CI job is failing due to completely unrelated issue (will be fixed by #68527), so it's now ready for review!

[kateinoigakukun]

preset=buildbot_incremental_linux_crosscompile_wasm
@swift-ci Please test with preset Linux Platform

[aschwaighofer]

Looks good to me

[kateinoigakukun]

@swift-ci Please smoke test and merge

[kateinoigakukun]

@swift-ci Please smoke test and merge