[IRGen] Replace duplicate code with new method #13720
Conversation
lib/AST/Decl.cpp
Outdated
| AccessLevel access = getEffectiveAccess(); | ||
| SILLinkage linkage; | ||
| switch (access) { | ||
| case AccessLevel::Private: |
There was a problem hiding this comment.
Formatting nit: ‘case’ should have the same level of indenting as ‘switch’ per standard LLVM/Swift project style.
lib/AST/Decl.cpp
Outdated
| @@ -46,6 +46,7 @@ | |||
| #include "swift/Basic/Range.h" | |||
| #include "swift/Basic/StringExtras.h" | |||
| #include "swift/Basic/Statistic.h" | |||
| #include "swift/SIL/SILLinkage.h" | |||
There was a problem hiding this comment.
AST should not depend on SIL. Instead, make this a global function in lib/SIL/SIL.cpp.
There was a problem hiding this comment.
Ok, that makes sense. I'll rework this.
There was a problem hiding this comment.
I force updated this change and moved the implementation to lib/SIL/SIL.cpp. include/SIL/SILModule.h seemed to be the best place for this declaration to live, but would it make sense for this to be a member function on SILModule directly?
lib/AST/Decl.cpp
Outdated
| switch (access) { | ||
| case AccessLevel::Private: | ||
| case AccessLevel::FilePrivate: | ||
| linkage = SILLinkage::Private; |
There was a problem hiding this comment.
This can just fold to 'return false'
lib/AST/Decl.cpp
Outdated
| linkage = SILLinkage::Private; | ||
| break; | ||
| case AccessLevel::Internal: | ||
| linkage = SILLinkage::Hidden; |
There was a problem hiding this comment.
This is where you return false if isWholeModule, otherwise true
lib/AST/Decl.cpp
Outdated
| break; | ||
| case AccessLevel::Public: | ||
| case AccessLevel::Open: | ||
| linkage = SILLinkage::Public; |
|
@slavapestov, what do you think of the latest version? |
| return false; | ||
| case AccessLevel::Public: |
There was a problem hiding this comment.
The comment says we should only consider private members, but it treats public members the same as internal members, which makes me think I'm possibly misunderstanding this.
There was a problem hiding this comment.
It's probably just an oversight.
There was a problem hiding this comment.
Ok. I think something in this later change broke a SILOptimizer test (global_property_opt.sil), but I wasn't yet able to figure out what it meant.
Here's a gist with the difference: https://gist.github.com/dtweston/1ce13a14a0cadbfe2088aa647ee7a891
There was a problem hiding this comment.
This test breakage has gone away, so I think everything's fine now.
| case AccessLevel::Private: | ||
| break; | ||
| } | ||
|
|
There was a problem hiding this comment.
The above code also appeared on lines 174 to 185, so it seems redundant to duplicate them here.
|
@dtweston Thank you! |
|
@swift-ci Please smoke test |
lib/SIL/SIL.cpp
Outdated
| @@ -115,3 +115,18 @@ swift::getLinkageForProtocolConformance(const NormalProtocolConformance *C, | |||
| return (definition ? SILLinkage::Public : SILLinkage::PublicExternal); | |||
| } | |||
| } | |||
|
|
|||
| bool isDeclVisibleExternally(const ValueDecl *decl, const SILModule *module) { | |||
There was a problem hiding this comment.
I think you to qualify the definition here, like bool swift::isDeclVisibleExternally(). CI failed to compile this PR with this error:
Undefined symbols for architecture x86_64:
"swift::isDeclVisibleExternally(swift::ValueDecl const*, swift::SILModule const*)", referenced from:
(anonymous namespace)::SILGlobalOpt::run() in libswiftSILOptimizer.a(GlobalOpt.cpp.o)
(anonymous namespace)::LetPropertiesOptPass::run() in libswiftSILOptimizer.a(LetPropertiesOpts.cpp.o)
(anonymous namespace)::LetPropertiesOpt::isConstantLetProperty(swift::VarDecl*) in libswiftSILOptimizer.a(LetPropertiesOpts.cpp.o)
(anonymous namespace)::GlobalPropertyOpt::getFieldEntry(swift::VarDecl*) in libswiftSILOptimizer.a(GlobalPropertyOpt.cpp.o)
(anonymous namespace)::DeadFunctionElimination::findAnchorsInTables() in libswiftSILOptimizer.a(DeadFunctionElimination.cpp.o)
isDefaultCaseKnown(swift::ClassHierarchyAnalysis*, swift::FullApplySite, swift::ClassDecl*, llvm::SmallVector<swift::ClassDecl*, 8u>&) in libswiftSILOptimizer.a(SpeculativeDevirtualizer.cpp.o)
isKnownFinalClass(swift::ClassDecl*, swift::SILModule&, swift::ClassHierarchyAnalysis*) in libswiftSILOptimizer.a(Devirtualize.cpp.o)
...
ld: symbol(s) not found for architecture x86_64
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
|
Hi @dtweston, it looks like the devirt-speculate test failure was real -- do you mind investigating? |
|
Closing due to age, but someone else can pick it up. |
This pull request removes duplicated code from
GlobalPropertyOpt::isVisibleExternally(),FunctionLivenessComputation::isVisibleExternally()and the static functionisAssignableExternally()inLetPropertiesOpts.cpp.Resolves SR-6264.