IRGen: Public non-resilient types that reference resilient types from the same module must view those types as non-fixed #25751
Conversation
|
@swift-ci Please test |
lib/IRGen/GenEnum.cpp
Outdated
| // is not fixed-size in all resilience domains that have knowledge of | ||
| // this enum's layout, we need to constrain our layout optimizations to | ||
| // what the runtime can reproduce. | ||
| if (!isResilient && !argTI->isFixedSize(layoutScope)) |
There was a problem hiding this comment.
I don’t understand why you’re adding the new lowering mode. It sounds like isFixedSize() returns true when it should return false?
There was a problem hiding this comment.
If you lower a resilient type in its local module it ends up as a fixed size type.
There was a problem hiding this comment.
Right but isFixedSize(ResilienceExpansion::Minimal) is meant to return false if it was fixed size in other resilience domains. It sounds like it’s not being computed correctly in all cases (nested enums?) and we need to fix the root cause instead of papering over it here.
There was a problem hiding this comment.
Sorry I mean “if it was not fixed size in other resilience domains”.
There was a problem hiding this comment.
The enum that we query in the test case (``enum ResilientType```) is not nested. I have to debug again but what I remember from yesterday is that if I get the type info for a resilient enum inside it's own module it returns a fixed size enum which always has fixed size.
I will confirm again that this is what happens.
There was a problem hiding this comment.
If you try the same example with a resilient struct instead of a resilient enum, do we correctly emit the payload size (and skip layout optimizations) for the non-resilient enum?
There was a problem hiding this comment.
Okay, I will look into that. Thanks.
There was a problem hiding this comment.
Yeah, only happens with the enum. I will track down where we miss setting the flag.
There was a problem hiding this comment.
Much simpler now.
|
Build failed |
|
Build failed |
rdar://51422528
aschwaighofer
force-pushed
the
irgen_resilient_non_resilient_same_module
branch
from
93e0cef to
6f4c5c4
Compare
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
lib/IRGen/GenEnum.cpp
Outdated
| @@ -5794,7 +5794,9 @@ std::unique_ptr<EnumImplStrategy> | |||
| EnumImplStrategy::get(TypeConverter &TC, SILType type, EnumDecl *theEnum) { | |||
| unsigned numElements = 0; | |||
| TypeInfoKind tik = Loadable; | |||
| IsFixedSize_t alwaysFixedSize = IsFixedSize; | |||
| IsFixedSize_t alwaysFixedSize = | |||
There was a problem hiding this comment.
I would prefer leaving the old line in place and adding a separate 'if' statement to set it to IsNotFixedSize, but that's up to you.
Thanks for figuring out the right fix! Looks like this has been broken ever since I added this code in 2015 🤦♂️
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
|
New version looks great, thank you very much! |
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
|
Unrelated error: |
|
@swift-ci Please test OS X platform |
Otherwise, we don't emit certain parts of the metadata that is expected
when these types are used outside the module (payload size).
rdar://51422528