[AST] NFC: Enable reference storage type meta-programming #16237
Conversation
davezarzycki
requested review from
xedin,
slavapestov,
jckarter,
gottesmm,
rjmccall,
DougGregor and
CodaFi
|
This passes validation testing on my local Linux box. Here goes: @swift-ci please clean test |
|
@swift-ci please clean test |
|
Build failed |
|
Helpful review notes:
@rjmccall – When it comes to IRGen, I'd appreciate it if you could pay close attention to the TypeInfo macros and especially the existential ones. In particular, the extra inhabitant value/mask logic. Also, please check @gottesmm and @jckarter – When it comes to SIL, I'd appreciate it if you guys could check the SIL optimization changes that I made. In particular, do any subtle switches over |
|
Build failed |
davezarzycki
force-pushed
the
metaprogram_ref_storage_types
branch
from
68ce370 to
9369dfa
Compare
|
@swift-ci please clean test |
|
Build failed |
|
Build failed |
|
|
||
| // NOTE: You will need to update ReferenceOwnership in ModuleFormat.h. | ||
| ADDRESS_ONLY_DYNAMIC_REF_STORAGE(Weak, weak, WEAK) | ||
| LOADABLE_DYNAMIC_REF_STORAGE(Unowned, unowned, UNOWNED) |
There was a problem hiding this comment.
It's only loadable for native classes.
There was a problem hiding this comment.
LOADABLE in this context means "load-able", not "always load".
There was a problem hiding this comment.
We use "loadable" to mean "values of this type can be loaded" pretty ubiquitously.
| #ifndef STATIC_REF_STORAGE | ||
| #define STATIC_REF_STORAGE(Name, name, NAME) \ | ||
| REF_STORAGE(Name, name, NAME) | ||
| #endif |
There was a problem hiding this comment.
Unmanaged has dynamic semantics. Those semantics may not add complicated extra code to copies and destroys, but they're definitely different.
I think what you're trying to express here is the level of runtime support they require, for synthesizing the runtime-functions list? Having a second metaprogramming database in IRGen isn't the end of the world if it helps this sort of thing.
There was a problem hiding this comment.
I expected this feedback. Do you have any better naming suggestions?
ADDRESS_ONLY_WITH_RUNTIME_LOGIC_REF_STORAGE?
LOADABLE_WITH_RUNTIME_SUPPORT_REF_STORAGE?
COMPILETIME_REF_STORAGE?
There was a problem hiding this comment.
What about the following?
ADDRESS_ONLY_CHECKED_REF_STORAGE(...)
LOADABLE_CHECKED_REF_STORAGE(...)
UNCHECKED_REF_STORAGE(...)
There was a problem hiding this comment.
Hey @rjmccall – I've been working on breaking up the pull request as you requested. I'd really appreciate it if we could nail down the names before I get too deep into the refactoring. Are you okay with the following?
NEVER_LOADABLE_CHECKED_REF_STORAGE(...) // "Weak" is an example
SOMETIMES_LOADABLE_CHECKED_REF_STORAGE(...) // "Unowned" is an example
ALWAYS_LOADABLE_CHECKED_REF_STORAGE(...) // No example from Swift, but I need it.
UNCHECKED_REF_STORAGE(...) // "Unmanaged" is an example
include/swift/AST/Ownership.h
Outdated
| #include "swift/AST/ReferenceStorage.def" | ||
| } | ||
| llvm_unreachable("impossible"); | ||
| } |
There was a problem hiding this comment.
Is strengthOf really necessary, or should it just be isStrongerThan?
There was a problem hiding this comment.
What I want is tristate comparison and returning int seemed like the easiest way to help clients do greater-than/less-than/equal-to comparisons.
There was a problem hiding this comment.
But why not just give them a comparator? It can return a non-bool if you want to do simultaneous comparison. But the numbers here are meaningless, and they define away partiality, which I expect will sometimes be the right result (should we really be "strengthening" weak references to unowned or vice-versa?).
| ATTRS(NoUnwind, FirstParamReturned)) | ||
| #define ADDRESS_ONLY_DYNAMIC_REF_STORAGE_HELPER(Name, Emit, SymName, HalfEmit) \ | ||
| /* void swift_##SymName##Destroy(Name##Reference *object); */ \ | ||
| FUNCTION(Emit##Name##Destroy, swift_##SymName##Destroy, C_CC, \ |
There was a problem hiding this comment.
Why Emit on all of these variable names?
There was a problem hiding this comment.
If you look at the use of the above macro, it is called twice, once with Emit set to Native and once with Emit set to Unknown.
There was a problem hiding this comment.
I see. Why would you call this parameter Emit? Maybe Nativeness?
lib/AST/Type.cpp
Outdated
| } \ | ||
| return ty->usesNativeReferenceCounting(resilience); \ | ||
| } | ||
| #include "swift/AST/ReferenceStorage.def" |
There was a problem hiding this comment.
This method only makes sense for UnownedStorageType. Weak references are never loadable and unmanaged references are always loadable.
There was a problem hiding this comment.
I have a research branch where I'm adding new reference storage types. This method needs to be able to deal with the new types without having hard coded knowledge about them.
There was a problem hiding this comment.
Oh wait, this method I found confusing. It is named isLoadable but it checks for native reference counting. If you're arguing that this is per ref-storage-type policy decision, then maybe the method should mimic the policy new APIs added to Ownership.h.
There was a problem hiding this comment.
I'm not sure that "unmanaged references are always loadable" is true. Before this pull request, there was no loadability check in the SIL verifier for Unmanaged (unlike Unowned), and trying to enable this assertion causes build regressions.
There was a problem hiding this comment.
I'm saying that most reference storage types do not have variable loadability; that is a special property of unowned references.
There was a problem hiding this comment.
Unmanaged is never not loadable. It's possible that we're messing that up somewhere.
There was a problem hiding this comment.
Great. Let's reconcile the loadability of Unmanaged separately. Please see: #16279
lib/IRGen/GenType.h
Outdated
| const LoadableTypeInfo *createUnmanagedStorageType(llvm::Type *valueType); | ||
| #define STATIC_REF_STORAGE(Name, ...) \ | ||
| const LoadableTypeInfo *create##Name##StorageType(llvm::Type *valueType); | ||
| #include "swift/AST/ReferenceStorage.def" |
There was a problem hiding this comment.
If it simplifies getting rid of STATIC_REF_STORAGE, I think it's probably fine for this to not return LoadableTypeInfo*.
lib/IRGen/HeapTypeInfo.h
Outdated
| out.add(IGF.Builder.CreatePtrToInt(value, getOptionalIntType())); \ | ||
| } else { \ | ||
| out.add(value); \ | ||
| } \ |
There was a problem hiding this comment.
This should be based on the actual optionality of the referent type, not whether the reference kind generally supports optionality. Otherwise it's not going to do the right thing for reference types whose referent type can be either optional or not.
There was a problem hiding this comment.
Thanks for clarifying. I wasn't sure about this and everything seemed to work with the code at hand.
How can one dig out the optionality state based on the data structures at hand? Is it doable? Or do I need to push an "is optional" boolean through the design?
There was a problem hiding this comment.
It probably wouldn't be the worst thing to pass a SILType to all the reference-counting operations, or yeah, you could store a boolean when creating it.
lib/Parse/ParseExpr.cpp
Outdated
| auto specifierKind = (ownershipKind != ReferenceOwnership::Weak) | ||
| ? VarDecl::Specifier::Let | ||
| : VarDecl::Specifier::Var; | ||
| auto specifierKind = optionalityOf(ownershipKind) == Optionality::Required |
There was a problem hiding this comment.
This is specific to weak, not to general optionality.
There was a problem hiding this comment.
Okay. I can change that.
|
Hey @rjmccall – As to your overall feedback about breaking up this patch, I can certainly do that as soon as we settle on the "right" macro names. |
davezarzycki
force-pushed
the
metaprogram_ref_storage_types
branch
from
aa94023 to
ed39561
Compare
|
@swift-ci please smoke test |
davezarzycki
force-pushed
the
metaprogram_ref_storage_types
branch
from
ed39561 to
839c106
Compare
davezarzycki
changed the title
|
Hi @rjmccall and everybody else. I've dialed the pull request way back and I'm starting a patch series that will involve multiple pull requests on a per-subsystem basis. I'd like to only seek sign off on the fundamental @swift-ci please smoke test |
|
Ping. Hi @rjmccall and everybody else. I've broken this pull request into a patch series as you requested and addressed more of your feedback. When you have the time, I'd appreciate it if you could approve the A question for the broader audience: would you prefer a pull request per subsystem? Or a series of independent patches on one branch? |
| enum : unsigned { NumReferenceOwnershipBits = | ||
| countBitsUsed(static_cast<unsigned>(ReferenceOwnership::Last_Kind)) }; | ||
|
|
||
| static inline llvm::StringRef keywordOf(ReferenceOwnership ownership) { |
There was a problem hiding this comment.
I'm not super happy with dumping all of these in the swift:: namespace. Maybe there should be a sub-namespace?
There was a problem hiding this comment.
This seems separable and larger than the pull request at hand. As the person to "blame" for many of these Num*Bits enums, I'd be happy to discuss in the forums what the "right" tradeoff is and then implement it. Do you want to start a thread?
There was a problem hiding this comment.
It's not the Num*Bits I care about, it's keywordOf and Optionality and things like that. I know the type signature shows what they're for, but this is still a header that gets included all over the place, and it's weird to put these generically-named declarations in the common namespace.
There was a problem hiding this comment.
I couldn't find any convention in Swift for how to workaround the fact that C++ doesn't allow methods on enum types. I'm open to suggestions.
In the case of this patch series and the way it is evolving, I could probably just move this logic to methods on ReferenceStorageType.
There was a problem hiding this comment.
I think we've just begrudgingly accepted the need to have fairly general-looking function names in the Swift namespace.
There was a problem hiding this comment.
Optionality in particular is really bad, compared to OptionalTypeKind.
There was a problem hiding this comment.
I'm not in love with the name either. I just want to abstract away the hardcoded knowledge about the "optionality" of the reference storage types. How about OwnershipOptionality? Or ReferenceStorageOptionality?
There was a problem hiding this comment.
Either of those is better, yeah. I think I'd lean towards the latter—this is not something that needs to be particularly concise.
There was a problem hiding this comment.
After some thought, I'm going with ReferenceOwnershipOptionality over ReferenceStorageOptionality. The rational is threefold:
- The default
ReferenceOwnershipis not a reference storage type. - An enum over just the reference storage types does not exist and I feel that there is not sufficient demand for such an enum to exist.
- I don't want to create a situation where the callers of
optionalityOf()need to check forStrongfirst before calling the API.
|
Hi @jrose-apple – I agree that the macros are "ugly" and I've been looking into ways to use templates instead of macros. The way that IRGen works though encourages some amount macro use because some type information has been lost. I've also contemplated other ideas, like lowering the reference storage types to one "SILReferenceStorage" type. I'm open to suggestions. I would also like to put forth the observation that this can't be cleaned up until the abstractions are disentangled from the specific reference storage types that exist at the moment; therefore the macros as ugly as they are represent "progress". |
|
I guess as long as other reviewers can picture where you're going, I'll stay out of it. |
|
I think those macro names are fine, thanks. |
davezarzycki
force-pushed
the
metaprogram_ref_storage_types
branch
from
839c106 to
b4b1dc7
Compare
This commit also fixes reference storage extra inhabitant bugs.
davezarzycki
force-pushed
the
metaprogram_ref_storage_types
branch
from
7760824 to
057bbb3
Compare
|
Hi @rjmccall – Sorry for the delay, I've been preparing my home for a newborn due mid August. I think this pull request is at a good merge point now. I've rebased to fix some SIL optimizer merge conflicts, and I've refactored the existential reference storage type logic to be more "best effort". I did need to fix a bug/oversight in SwiftTargetInfo.cpp to make it work though. Review would be appreciated. @swift-ci please test |
|
Build failed |
|
Build failed |
|
Let's try again now that apple/swift-lldb#722 has been merged. @swift-ci please test |
|
Build failed |
|
Build failed |
|
Looks like CI failed. Let's try again: @swift-ci please test |
| #define REF_STORAGE(Name, ...) \ | ||
| void set##Name() { Data |= Name; } \ | ||
| bool is##Name() const { return Data & Name; } | ||
| #include "swift/AST/ReferenceStorage.def" |
There was a problem hiding this comment.
Do you want to metaprogram the actual flags, too? That's okay if this isn't ABI-exposed.
There was a problem hiding this comment.
I don't think there's actually a reason for this to be using a manual bitmask anyway instead of bit-fields.
There was a problem hiding this comment.
The assignment of a new reference storage type to a flag is not meta-programmed, but the fallout from adding a new flag is. In other words, the build will fail until a programmer manually updates the enum within TypeReferenceOwnership at the top of stdlib/public/runtime/Private.h. I can't see a problem with this, can you?
|
Otherwise this looks good; the test changes all seem acceptable, and the change to SwiftTargetInfo.cpp is fine. |
|
Great! If no more changes are needed, can we merge this? Also, as a follow up, given that this pull request series makes optional Unowned/Unmanaged possible (but not enabled) in the user facing language, what should be done about that? Should we just "flip the switch" in a later pull request? Or does this need to go through swift evolution? |
|
Hi @rjmccall – Just a "ping". Can we merge this? It'd be nice to do so before the next and inevitable merge conflict. |
|
Merged. It's fine to just do optional unowned types as a follow-up PR; it doesn't need to go through evolution. |
|
@rjmccall – Thank you so much for your time an patience in reviewing this large pull request. I owe you a beer or three the next time we hang out :-) |
|
@davezarzycki this PR introduced a huge number of warnings in no-asserts builds because it asserts on theoretically unreachable cases, but in no-assert those cases fall through to the next case. I raised #17770 but for big PRs it's worth always running a no-assert build as well. |
|
My mistake. As I pointed out in #17770, using |
|
@davezarzycki This broke the source compatibility suite -- ReactiveCocoa now fails to build with a SILGen assertion: |
This fixes a regression from swiftlang#16237.
John okayed this change in a comment on GitHub pull request: swiftlang#16237
Adding new reference storage types is mostly boilerplate. This patch series makes adding new reference storage types easier.