-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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.
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
SIL: Tiny conformance substitution cleanups #80594
base: main
Are you sure you want to change the base?
SIL: Tiny conformance substitution cleanups #80594
Conversation
@@ -71,9 +71,8 @@ public struct WitnessTable : CustomStringConvertible, NoReflectionChildren { | |||
OptionalBridgedFunction(obj: witness?.bridged.obj)) | |||
case .associatedType(let requirement, let witness): | |||
return BridgedWitnessTableEntry.createAssociatedType(requirement.bridged, witness.bridged) | |||
case .associatedConformance(let requirement, let substType, let witness): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eeckstein I was lazy but the substType
tuple element can be removed from this enum case entirely now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove the enum case in this PR? Shouldn't be too much work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. (it's a tuple field in an enum case, not the whole enum case that I removed.)
5bb42c3
to
99cd4bd
Compare
return; | ||
makeAlive(WT); | ||
|
||
// FIXME: If 'concrete' is a SpecializedProtocolConformance, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eeckstein Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually doesn't matter because we don't do dead witnesstable elimination. In findAnchorsInTables
we set all witness tables alive. We had to do this because it's almost impossible to find all references to witness tables in SIL. Only IRGen knows that - and we emit witness tables lazily there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. I'll just delete the logic then since it's pointless to keep around code that is unused and incorrect.
@@ -71,9 +71,8 @@ public struct WitnessTable : CustomStringConvertible, NoReflectionChildren { | |||
OptionalBridgedFunction(obj: witness?.bridged.obj)) | |||
case .associatedType(let requirement, let witness): | |||
return BridgedWitnessTableEntry.createAssociatedType(requirement.bridged, witness.bridged) | |||
case .associatedConformance(let requirement, let substType, let witness): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove the enum case in this PR? Shouldn't be too much work.
return; | ||
makeAlive(WT); | ||
|
||
// FIXME: If 'concrete' is a SpecializedProtocolConformance, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually doesn't matter because we don't do dead witnesstable elimination. In findAnchorsInTables
we set all witness tables alive. We had to do this because it's almost impossible to find all references to witness tables in SIL. Only IRGen knows that - and we emit witness tables lazily there.
0625b16
to
f7fc125
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks!
This logic was unconditionally disabled and incorrect.
f7fc125
to
be5d03f
Compare
@swift-ci Please smoke test |
@swift-ci Please test source compatibility |
@swift-ci benchmark |
Follow-up to #80482 that removes origType plumbing from a few more places in SIL.