Skip to content

Centralize KeyPath accessor calling convention logic to IRGen #66273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 20, 2023
+1,058 −523
Merged

Centralize KeyPath accessor calling convention logic to IRGen #66273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 76 additions & 10 deletions include/swift/AST/ExtInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,15 @@ enum class SILFunctionTypeRepresentation : uint8_t {
/// constructor). Except for
/// handling the "this" argument, has the same behavior as "CFunctionPointer".
CXXMethod,

/// A KeyPath accessor function, which is thin and also uses the variadic
/// length generic components serialization in trailing buffer.
/// Each representation has a different convention for which parameters
/// have serialized generic type info.
KeyPathAccessorGetter,
KeyPathAccessorSetter,
KeyPathAccessorEquals,
KeyPathAccessorHash,
Comment on lines +181 to +184
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙏

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jckarter jckarter Jun 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@kateinoigakukun kateinoigakukun Jun 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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) 🙏

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jckarter gentle ping :)

};

/// Returns true if the function with this convention doesn't carry a context.
Expand Down Expand Up @@ -203,6 +212,10 @@ isThinRepresentation(SILFunctionTypeRepresentation rep) {
case SILFunctionTypeRepresentation::CFunctionPointer:
case SILFunctionTypeRepresentation::Closure:
case SILFunctionTypeRepresentation::CXXMethod:
case SILFunctionTypeRepresentation::KeyPathAccessorGetter:
case SILFunctionTypeRepresentation::KeyPathAccessorSetter:
case SILFunctionTypeRepresentation::KeyPathAccessorEquals:
case SILFunctionTypeRepresentation::KeyPathAccessorHash:
return true;
}
llvm_unreachable("Unhandled SILFunctionTypeRepresentation in switch.");
Expand All @@ -215,6 +228,31 @@ isThickRepresentation(Repr repr) {
return !isThinRepresentation(repr);
}

/// Returns true if the function with this convention receives generic arguments
/// from KeyPath argument buffer.
constexpr bool
isKeyPathAccessorRepresentation(SILFunctionTypeRepresentation rep) {
switch (rep) {
case SILFunctionTypeRepresentation::KeyPathAccessorGetter:
case SILFunctionTypeRepresentation::KeyPathAccessorSetter:
case SILFunctionTypeRepresentation::KeyPathAccessorEquals:
case SILFunctionTypeRepresentation::KeyPathAccessorHash:
return true;
case SILFunctionTypeRepresentation::Thick:
case SILFunctionTypeRepresentation::Block:
case SILFunctionTypeRepresentation::Thin:
case SILFunctionTypeRepresentation::Method:
case SILFunctionTypeRepresentation::ObjCMethod:
case SILFunctionTypeRepresentation::WitnessMethod:
case SILFunctionTypeRepresentation::CFunctionPointer:
case SILFunctionTypeRepresentation::Closure:
case SILFunctionTypeRepresentation::CXXMethod:
return false;
}
llvm_unreachable("Unhandled SILFunctionTypeRepresentation in switch.");
}


constexpr SILFunctionTypeRepresentation
convertRepresentation(FunctionTypeRepresentation rep) {
switch (rep) {
Expand Down Expand Up @@ -246,6 +284,10 @@ convertRepresentation(SILFunctionTypeRepresentation rep) {
case SILFunctionTypeRepresentation::ObjCMethod:
case SILFunctionTypeRepresentation::WitnessMethod:
case SILFunctionTypeRepresentation::Closure:
case SILFunctionTypeRepresentation::KeyPathAccessorGetter:
case SILFunctionTypeRepresentation::KeyPathAccessorSetter:
case SILFunctionTypeRepresentation::KeyPathAccessorEquals:
case SILFunctionTypeRepresentation::KeyPathAccessorHash:
return llvm::None;
}
llvm_unreachable("Unhandled SILFunctionTypeRepresentation!");
Expand All @@ -265,6 +307,10 @@ constexpr bool canBeCalledIndirectly(SILFunctionTypeRepresentation rep) {
case SILFunctionTypeRepresentation::ObjCMethod:
case SILFunctionTypeRepresentation::Method:
case SILFunctionTypeRepresentation::WitnessMethod:
case SILFunctionTypeRepresentation::KeyPathAccessorGetter:
case SILFunctionTypeRepresentation::KeyPathAccessorSetter:
case SILFunctionTypeRepresentation::KeyPathAccessorEquals:
case SILFunctionTypeRepresentation::KeyPathAccessorHash:
return true;
}

Expand All @@ -286,6 +332,10 @@ template <typename Repr> constexpr bool shouldStoreClangType(Repr repr) {
case SILFunctionTypeRepresentation::Method:
case SILFunctionTypeRepresentation::WitnessMethod:
case SILFunctionTypeRepresentation::Closure:
case SILFunctionTypeRepresentation::KeyPathAccessorGetter:
case SILFunctionTypeRepresentation::KeyPathAccessorSetter:
case SILFunctionTypeRepresentation::KeyPathAccessorEquals:
case SILFunctionTypeRepresentation::KeyPathAccessorHash:
return false;
}
llvm_unreachable("Unhandled SILFunctionTypeRepresentation.");
Expand Down Expand Up @@ -396,6 +446,10 @@ class ASTExtInfoBuilder {
case SILFunctionTypeRepresentation::Thin:
case SILFunctionTypeRepresentation::CFunctionPointer:
case SILFunctionTypeRepresentation::Closure:
case SILFunctionTypeRepresentation::KeyPathAccessorGetter:
case SILFunctionTypeRepresentation::KeyPathAccessorSetter:
case SILFunctionTypeRepresentation::KeyPathAccessorEquals:
case SILFunctionTypeRepresentation::KeyPathAccessorHash:
return false;
case SILFunctionTypeRepresentation::ObjCMethod:
case SILFunctionTypeRepresentation::Method:
Expand Down Expand Up @@ -634,6 +688,10 @@ SILFunctionLanguage getSILFunctionLanguage(SILFunctionTypeRepresentation rep) {
case SILFunctionTypeRepresentation::Method:
case SILFunctionTypeRepresentation::WitnessMethod:
case SILFunctionTypeRepresentation::Closure:
case SILFunctionTypeRepresentation::KeyPathAccessorGetter:
case SILFunctionTypeRepresentation::KeyPathAccessorSetter:
case SILFunctionTypeRepresentation::KeyPathAccessorEquals:
case SILFunctionTypeRepresentation::KeyPathAccessorHash:
return SILFunctionLanguage::Swift;
}

Expand All @@ -652,20 +710,20 @@ class SILExtInfoBuilder {
// and NumMaskBits must be updated, and they must match.

// |representation|pseudogeneric| noescape | concurrent | async
// | 0 .. 3 | 4 | 5 | 6 | 7
// | 0 .. 4 | 5 | 6 | 7 | 8
// |differentiability|unimplementable|
// | 8 .. 10 | 11 |
// | 9 .. 11 | 12 |
//
enum : unsigned {
RepresentationMask = 0xF << 0,
PseudogenericMask = 1 << 4,
NoEscapeMask = 1 << 5,
SendableMask = 1 << 6,
AsyncMask = 1 << 7,
DifferentiabilityMaskOffset = 8,
RepresentationMask = 0x1F << 0,
PseudogenericMask = 1 << 5,
NoEscapeMask = 1 << 6,
SendableMask = 1 << 7,
AsyncMask = 1 << 8,
DifferentiabilityMaskOffset = 9,
DifferentiabilityMask = 0x7 << DifferentiabilityMaskOffset,
UnimplementableMask = 1 << 11,
NumMaskBits = 12
UnimplementableMask = 1 << 12,
NumMaskBits = 13
};

unsigned bits; // Naturally sized for speed.
Expand Down Expand Up @@ -765,6 +823,10 @@ class SILExtInfoBuilder {
case Representation::Thin:
case Representation::CFunctionPointer:
case Representation::Closure:
case Representation::KeyPathAccessorGetter:
case Representation::KeyPathAccessorSetter:
case Representation::KeyPathAccessorEquals:
case Representation::KeyPathAccessorHash:
return false;
case Representation::ObjCMethod:
case Representation::Method:
Expand All @@ -788,6 +850,10 @@ class SILExtInfoBuilder {
case Representation::WitnessMethod:
case Representation::Closure:
case SILFunctionTypeRepresentation::CXXMethod:
case Representation::KeyPathAccessorGetter:
case Representation::KeyPathAccessorSetter:
case Representation::KeyPathAccessorEquals:
case Representation::KeyPathAccessorHash:
return false;
}
llvm_unreachable("Unhandled Representation in switch.");
Expand Down
2 changes: 1 addition & 1 deletion include/swift/AST/Types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ class alignas(1 << TypeAlignInBits) TypeBase

protected:
enum { NumAFTExtInfoBits = 11 };
enum { NumSILExtInfoBits = 12 };
enum { NumSILExtInfoBits = 13 };

// clang-format off
union { uint64_t OpaqueBits;
Expand Down
5 changes: 5 additions & 0 deletions include/swift/SIL/ApplySite.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#ifndef SWIFT_SIL_APPLYSITE_H
#define SWIFT_SIL_APPLYSITE_H

#include "swift/AST/ExtInfo.h"
#include "swift/Basic/STLExtras.h"
#include "swift/SIL/SILArgument.h"
#include "swift/SIL/SILBasicBlock.h"
Expand Down Expand Up @@ -259,6 +260,10 @@ class ApplySite {
case SILFunctionTypeRepresentation::ObjCMethod:
case SILFunctionTypeRepresentation::WitnessMethod:
case SILFunctionTypeRepresentation::Closure:
case SILFunctionTypeRepresentation::KeyPathAccessorGetter:
case SILFunctionTypeRepresentation::KeyPathAccessorSetter:
case SILFunctionTypeRepresentation::KeyPathAccessorEquals:
case SILFunctionTypeRepresentation::KeyPathAccessorHash:
return true;
case SILFunctionTypeRepresentation::Block:
case SILFunctionTypeRepresentation::Thick:
Expand Down
8 changes: 8 additions & 0 deletions lib/AST/ASTDumper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,14 @@ static StringRef getDumpString(SILFunctionType::Representation value) {
case SILFunctionType::Representation::ObjCMethod: return "objc_method";
case SILFunctionType::Representation::WitnessMethod: return "witness_method";
case SILFunctionType::Representation::Closure: return "closure";
case SILFunctionType::Representation::KeyPathAccessorGetter:
return "keypath_accessor_getter";
case SILFunctionType::Representation::KeyPathAccessorSetter:
return "keypath_accessor_setter";
case SILFunctionType::Representation::KeyPathAccessorEquals:
return "keypath_accessor_equals";
case SILFunctionType::Representation::KeyPathAccessorHash:
return "keypath_accessor_hash";
}

llvm_unreachable("Unhandled SILFunctionTypeRepresentation in switch.");
Expand Down
7 changes: 7 additions & 0 deletions lib/AST/ASTMangler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2038,6 +2038,13 @@ void ASTMangler::appendImplFunctionType(SILFunctionType *fn,
case SILFunctionTypeRepresentation::WitnessMethod:
OpArgs.push_back('W');
break;
case SILFunctionTypeRepresentation::KeyPathAccessorGetter:
case SILFunctionTypeRepresentation::KeyPathAccessorSetter:
case SILFunctionTypeRepresentation::KeyPathAccessorEquals:
case SILFunctionTypeRepresentation::KeyPathAccessorHash:
// KeyPath accessors are mangled separately based on their index types
// by mangleKeyPathGetterThunkHelper, and so on.
llvm_unreachable("key path accessors should not mangle its function type");
}

// Coroutine kind. This is mangled in all pointer auth modes.
Expand Down
24 changes: 24 additions & 0 deletions lib/AST/ASTPrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6507,6 +6507,18 @@ class TypePrinter : public TypeVisitor<TypePrinter> {
case SILFunctionType::Representation::Closure:
Printer << "closure";
break;
case SILFunctionType::Representation::KeyPathAccessorGetter:
Printer << "keypath_accessor_getter";
break;
case SILFunctionType::Representation::KeyPathAccessorSetter:
Printer << "keypath_accessor_setter";
break;
case SILFunctionType::Representation::KeyPathAccessorEquals:
Printer << "keypath_accessor_equals";
break;
case SILFunctionType::Representation::KeyPathAccessorHash:
Printer << "keypath_accessor_hash";
break;
}
Printer << ")";
Printer.printStructurePost(PrintStructureKind::BuiltinAttribute);
Expand Down Expand Up @@ -6591,6 +6603,18 @@ class TypePrinter : public TypeVisitor<TypePrinter> {
case SILFunctionType::Representation::Closure:
Printer << "closure";
break;
case SILFunctionType::Representation::KeyPathAccessorGetter:
Printer << "keypath_accessor_getter";
break;
case SILFunctionType::Representation::KeyPathAccessorSetter:
Printer << "keypath_accessor_setter";
break;
case SILFunctionType::Representation::KeyPathAccessorEquals:
Printer << "keypath_accessor_equals";
break;
case SILFunctionType::Representation::KeyPathAccessorHash:
Printer << "keypath_accessor_hash";
break;
}
Printer << ")";
Printer.printStructurePost(PrintStructureKind::BuiltinAttribute);
Expand Down
4 changes: 4 additions & 0 deletions lib/AST/ClangTypeConverter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,10 @@ ClangTypeConverter::getFunctionType(ArrayRef<SILParameterInfo> params,
case SILFunctionType::Representation::ObjCMethod:
case SILFunctionType::Representation::WitnessMethod:
case SILFunctionType::Representation::Closure:
case SILFunctionType::Representation::KeyPathAccessorGetter:
case SILFunctionType::Representation::KeyPathAccessorSetter:
case SILFunctionType::Representation::KeyPathAccessorEquals:
case SILFunctionType::Representation::KeyPathAccessorHash:
llvm_unreachable("Expected a C-compatible representation.");
}
llvm_unreachable("unhandled representation!");
Expand Down
9 changes: 8 additions & 1 deletion lib/IRGen/CallEmission.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@
#ifndef SWIFT_IRGEN_CALLEMISSION_H
#define SWIFT_IRGEN_CALLEMISSION_H

#include "Temporary.h"
#include "Address.h"
#include "Callee.h"
#include "Temporary.h"

namespace llvm {
class CallSite;
Expand Down Expand Up @@ -49,6 +50,10 @@ class CallEmission {
/// Temporaries required by the call.
TemporarySet Temporaries;

/// Temporaries required by the call that are not mapped to any
/// SIL value.
SmallVector<StackAddress, 8> RawTempraries;

/// The function we're going to call.
Callee CurCallee;

Expand Down Expand Up @@ -78,6 +83,8 @@ class CallEmission {
virtual void emitCallToUnmappedExplosion(llvm::CallBase *call,
Explosion &out) = 0;
void emitYieldsToExplosion(Explosion &out);
void setKeyPathAccessorArguments(Explosion &in, bool isOutlined,
Explosion &out);
virtual FunctionPointer getCalleeFunctionPointer() = 0;
llvm::CallBase *emitCallSite();

Expand Down
5 changes: 5 additions & 0 deletions lib/IRGen/Callee.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ namespace irgen {
TaskGroupWaitNext,
TaskGroupWaitAll,
DistributedExecuteTarget,
KeyPathAccessor,
};

private:
Expand Down Expand Up @@ -260,6 +261,7 @@ namespace irgen {
case SpecialKind::TaskGroupWaitAll:
return true;
case SpecialKind::DistributedExecuteTarget:
case SpecialKind::KeyPathAccessor:
return false;
}
llvm_unreachable("covered switch");
Expand Down Expand Up @@ -289,6 +291,9 @@ namespace irgen {
case SpecialKind::AsyncLetFinish:
case SpecialKind::TaskGroupWaitNext:
case SpecialKind::TaskGroupWaitAll:
// KeyPath accessor functions receive their generic arguments
// as part of indices buffer.
case SpecialKind::KeyPathAccessor:
return true;
case SpecialKind::DistributedExecuteTarget:
return false;
Expand Down
Loading