-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[5.5] Fix two cross-module-optimization bugs #39018
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -406,7 +406,14 @@ SILFunction *SILDeserializer::getFuncForReference(StringRef name, | |
// SIL. | ||
SourceLoc sourceLoc; | ||
SILSerializationFunctionBuilder builder(SILMod); | ||
return builder.createDeclaration(name, type, RegularLocation(sourceLoc)); | ||
fn = builder.createDeclaration(name, type, | ||
RegularLocation(sourceLoc)); | ||
// The function is not really de-serialized, but it's important to call | ||
// `didDeserialize` on every new function. Otherwise some Analysis might miss | ||
// `notifyAddedOrModifiedFunction` notifications. | ||
if (Callback) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this not in the builder itself? A user should not have to worry about this sort of thing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's how you implemented it 😊 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. I'll take that. More than 2 weeks = p. That being said, we should consider adding it into the Builder (not in this commit though). I think in the SILOptFunctionBuilder I did this and maybe just didn't get around to doing this with this specific builder. |
||
Callback->didDeserialize(MF->getAssociatedModule(), fn); | ||
return fn; | ||
} | ||
|
||
/// Helper function to find a SILFunction, given its name and type. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
|
||
#include "c-module.h" | ||
|
||
long privateCFunc() { | ||
return 123; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
|
||
long privateCFunc(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,3 +12,6 @@ public struct PrivateStr { | |
} | ||
} | ||
|
||
public func privateFunc() -> Int { | ||
return 40 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
module PrivateCModule { | ||
header "c-module.h" | ||
} |
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.
This looks reasonable to me. But I would like someone who understands the import cache to also look at this. I just don't know how it works.
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.
I added @xymus who also reviewed the original code