Skip to content
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

Jump to bottom

IRGen: Use any field of fixed-layout structs for extra inhabitants. #18630

Merged
merged 1 commit into from
Aug 27, 2018
Merged

IRGen: Use any field of fixed-layout structs for extra inhabitants. #18630

merged 1 commit into from
Aug 27, 2018

Conversation

jckarter
Copy link
Contributor

@jckarter jckarter commented Aug 10, 2018 •
edited
Loading

This allows us to layout-optimize Optional when T is a struct with an extra-inhabitant-bearing field anywhere in its definition, not only at the beginning. Since runtime struct layout instantiation does not yet handle this, limit the effect only to structs whose layout is always completely known at compile time independent of their type parameters. rdar://problem/19690160

@jckarter
Copy link
Contributor Author

@swift-ci Please test

@jckarter
Copy link
Contributor Author

@swift-ci Please test source compatibility

@jckarter
Copy link
Contributor Author

@slavapestov Is there anywhere in RemoteAST/RemoteMirror or lldb that extra inhabitant counting for structs is hardcoded that I should update?

dcci
dcci reviewed Aug 10, 2018
View reviewed changes
Copy link
Member

@dcci dcci left a comment

Choose a reason for hiding this comment

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

@jckarter do you have an example where the layout of the struct changed before/after your change?
If so, I can try in lldb and let you know if there's anything broken there.

@milseman
Copy link
Member

This is great and gets us a fair deal closer to optimal extra-inhabitant utilization!

@jckarter
Copy link
Contributor Author

@dcci It would be different in a case where there's an object reference in a struct in some position other than first, and you use an Optional of that struct. For instance:

struct Test {
  var a: Int
  var b: AnyObject
}

Previously, Optional<Test> would use an extra byte for its tag. With this change, Optional<Test> is the same size, and uses a null pointer in b to represent nil.

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 5cfc15071261ef2360e8d06ab1483cac6c511de4

@jckarter jckarter force-pushed the extra-inhabitants-from-any-fixed-layout-struct-field branch from 5cfc150 to 8bcab80 Compare August 10, 2018 19:43
@jckarter
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 5cfc15071261ef2360e8d06ab1483cac6c511de4

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 5cfc15071261ef2360e8d06ab1483cac6c511de4

rjmccall
rjmccall approved these changes Aug 10, 2018
View reviewed changes
Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

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

LGTM.

@jckarter
Copy link
Contributor Author

@swift-ci Please test source compatibility

1 similar comment
@jckarter
Copy link
Contributor Author

@swift-ci Please test source compatibility

@slavapestov
Copy link
Contributor

@jckarter Remote mirrors do not know if the layout was static or dynamic. Look at stdlib/public/Reflection/TypeLowering.cpp.

Also what about resilience? If the struct is resilient, you can't use its extra inhabitants with your scheme.

I would suggest implementing the runtime support at the same time and getting rid of the special case. Other than that, this is a great idea!

slavapestov
slavapestov requested changes Aug 10, 2018
View reviewed changes
Copy link
Contributor

@slavapestov slavapestov left a comment

Choose a reason for hiding this comment

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

Yeah, I don't think this is sound unfortunately. We need compile time and runtime to always produce the same result, even if some fields are non-fixed at compile time (or are fixed in the current module but resilient outside).

lib/IRGen/GenStruct.cpp Outdated

for (auto &field : fields) {
auto *ti = dyn_cast<FixedTypeInfo>(&field.getTypeInfo());
// If any field is non-fixed, we can't definitively pick a best one.
Copy link
Contributor

Choose a reason for hiding this comment

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

The correct check would be isAlwaysFixedSize(), which takes all resilience domains into account; otherwise, this will produce different results across resilience boundaries. However, there's no way currently to replicate that check at runtime.

Could we just call the dynamic extra inhabitant value witness functions in this case?

slavapestov
slavapestov reviewed Aug 10, 2018
View reviewed changes
lib/IRGen/GenType.cpp Outdated
@@ -1843,7 +1843,8 @@ TypeCacheEntry TypeConverter::convertAnyNominalType(CanType type,
case DeclKind::Enum:
return convertEnumType(type.getPointer(), type, cast<EnumDecl>(decl));
case DeclKind::Struct:
return convertStructType(type.getPointer(), type, cast<StructDecl>(decl));
return convertStructType(type.getPointer(), type, cast<StructDecl>(decl),
/*is type dependent*/ decl->isGenericContext());
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't actually mean its dependent, eg struct Foo<T> { var x: Int }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That condition is checked further up before we enter this switch. I agree it's best to just do the runtime side of this though.

@jckarter
IRGen: Use any field of structs for extra inhabitants.
9f02ecd 
This allows us to layout-optimize Optional<T> when T is a struct with an
extra-inhabitant-bearing field anywhere in its definition, not only at
the beginning. rdar://problem/43019427
@jckarter jckarter force-pushed the extra-inhabitants-from-any-fixed-layout-struct-field branch from 33de71d to 9f02ecd Compare August 14, 2018 19:53
@jckarter
Copy link
Contributor Author

@swift-ci Please test

@jckarter
Copy link
Contributor Author

jckarter commented Aug 14, 2018 •
edited
Loading

@rjmccall @slavapestov I added handling for a runtime-selected extra inhabitant field as well. How does this look?

The annoying conditional dance here has me thinking we probably ought to just have a non-executable representation of the extra inhabitant set, since all of our extra-inhabitant-bearing types' sets are representable as an (offset, width, starting value, stride) tuple. That would let us shed those value witnesses altogether, and would make forwarding the extra inhabitants from the chosen field at runtime here trivial. I'd like to investigate that separately from this PR, though.

@swiftlang swiftlang deleted a comment from swift-ci Aug 14, 2018
@swiftlang swiftlang deleted a comment from swift-ci Aug 14, 2018
rjmccall
rjmccall approved these changes Aug 16, 2018
View reviewed changes
Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

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

Thanks, I think this is a lot better.

@jckarter jckarter merged commit 75d9e56 into swiftlang:master Aug 27, 2018
@bob-wilson bob-wilson mentioned this pull request Sep 25, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcci dcci dcci left review comments

@slavapestov slavapestov slavapestov requested changes

@rjmccall rjmccall rjmccall approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jckarter @milseman @swift-ci @slavapestov @rjmccall @dcci