Skip to content

SILOptimizer: make vtable pruning less aggressive #33770

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

Merged
merged 1 commit into from
Sep 4, 2020
Merged

SILOptimizer: make vtable pruning less aggressive #33770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions lib/SILOptimizer/Transforms/PruneVTables.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,16 @@ using namespace swift;

namespace {
class PruneVTables : public SILModuleTransform {
void runOnVTable(SILModule *M,
SILVTable *vtable) {
void runOnVTable(SILModule *M, SILVTable *vtable) {
LLVM_DEBUG(llvm::dbgs() << "PruneVTables inspecting table:\n";
vtable->print(llvm::dbgs()));
if (!M->isWholeModule() &&
vtable->getClass()->getEffectiveAccess() >= AccessLevel::FilePrivate) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be > not >= then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, I thought Id fixed that. #33808

Copy link
Contributor

@jckarter jckarter Sep 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think this change is unnecessarily pessimistic. The check for calleesAreStaticallyKnowable below should already prevent us from pruning nonfinal internal vtable entries. We should still be able to prune entries for final methods as long as the final method is not itself an override, because it should never need a vtable entry for originally-final methods to begin with, and we can reliably check whether a method is final across modules without an optimization pass. I suppose that means we could set the nonoverridden bit in SILGen for final methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like if a method is originally final we already don't give it a vtable entry at all. So, we should be able to remove the if (!methodDecl->isFinal()) precondition below and get the same effect. I think that's better than reimplementing calleesAreStaticallyKnowable here.

LLVM_DEBUG(llvm::dbgs() << "Ignoring visible table: ";
vtable->print(llvm::dbgs()));
return;
}

for (auto &entry : vtable->getMutableEntries()) {

// We don't need to worry about entries that are overridden,
Expand Down
20 changes: 10 additions & 10 deletions test/SILOptimizer/prune-vtables.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ sil @PrivateA_yesOverrides : $@convention(method) (@guaranteed PrivateA) -> ()
sil @PrivateA_isFinal : $@convention(method) (@guaranteed PrivateA) -> ()

// NOWMO-LABEL: sil_vtable PrivateA {
// NOWMO: #PrivateA.noOverrides{{.*}} [nonoverridden]
// NOWMO: #PrivateA.noOverrides{{[^[]]*}}
// NOWMO-NOT: #PrivateA.yesOverrides{{.*}} [nonoverridden]
// NOWMO: #PrivateA.isFinal{{.*}} [nonoverridden]
// NOWMO: #PrivateA.isFinal{{[^[]]*}}

// WMO-LABEL: sil_vtable PrivateA {
// WMO: #PrivateA.noOverrides{{.*}} [nonoverridden]
Expand All @@ -35,9 +35,9 @@ private class PrivateB: PrivateA {
sil @PrivateB_yesOverrides : $@convention(method) (@guaranteed PrivateB) -> ()

// NOWMO-LABEL: sil_vtable PrivateB {
// NOWMO: #PrivateA.noOverrides{{.*}} [nonoverridden]
// NOWMO: #PrivateA.noOverrides{{[^[]]*}}
// NOWMO-NOT: #PrivateA.yesOverrides{{.*}} [nonoverridden]
// NOWMO: #PrivateA.isFinal{{.*}} [nonoverridden]
// NOWMO: #PrivateA.isFinal{{[^[]]*}}

// WMO-LABEL: sil_vtable PrivateB {
// WMO: #PrivateA.noOverrides{{.*}} [nonoverridden]
Expand All @@ -62,7 +62,7 @@ sil @InternalA_isFinal : $@convention(method) (@guaranteed InternalA) -> ()
// NOWMO-LABEL: sil_vtable InternalA {
// NOWMO-NOT: #InternalA.noOverrides{{.*}} [nonoverridden]
// NOWMO-NOT: #InternalA.yesOverrides{{.*}} [nonoverridden]
// NOWMO: #InternalA.isFinal{{.*}} [nonoverridden]
// NOWMO: #InternalA.isFinal{{[^[]]*}}

// WMO-LABEL: sil_vtable InternalA {
// WMO: #InternalA.noOverrides{{.*}} [nonoverridden]
Expand All @@ -83,7 +83,7 @@ sil @InternalB_yesOverrides : $@convention(method) (@guaranteed InternalB) -> ()
// NOWMO-LABEL: sil_vtable InternalB {
// NOWMO-NOT: #InternalA.noOverrides{{.*}} [nonoverridden]
// NOWMO-NOT: #InternalA.yesOverrides{{.*}} [nonoverridden]
// NOWMO: #InternalA.isFinal{{.*}} [nonoverridden]
// NOWMO: #InternalA.isFinal{{[^[]]*}}

// WMO-LABEL: sil_vtable InternalB {
// WMO: #InternalA.noOverrides{{.*}} [nonoverridden]
Expand All @@ -108,7 +108,7 @@ sil @PublicA_isFinal : $@convention(method) (@guaranteed PublicA) -> ()
// NOWMO-LABEL: sil_vtable PublicA {
// NOWMO-NOT: #PublicA.noOverrides{{.*}} [nonoverridden]
// NOWMO-NOT: #PublicA.yesOverrides{{.*}} [nonoverridden]
// NOWMO: #PublicA.isFinal{{.*}} [nonoverridden]
// NOWMO: #PublicA.isFinal{{[^[]]*}}

// WMO-LABEL: sil_vtable PublicA {
// WMO: #PublicA.noOverrides{{.*}} [nonoverridden]
Expand All @@ -129,7 +129,7 @@ sil @PublicB_yesOverrides : $@convention(method) (@guaranteed PublicB) -> ()
// NOWMO-LABEL: sil_vtable PublicB {
// NOWMO-NOT: #PublicA.noOverrides{{.*}} [nonoverridden]
// NOWMO-NOT: #PublicA.yesOverrides{{.*}} [nonoverridden]
// NOWMO: #PublicA.isFinal{{.*}} [nonoverridden]
// NOWMO: #PublicA.isFinal{{[^[]]*}}

// WMO-LABEL: sil_vtable PublicB {
// WMO: #PublicA.noOverrides{{.*}} [nonoverridden]
Expand All @@ -154,7 +154,7 @@ sil @OpenA_isFinal : $@convention(method) (@guaranteed OpenA) -> ()
// NOWMO-LABEL: sil_vtable OpenA {
// NOWMO-NOT: #OpenA.noOverrides{{.*}} [nonoverridden]
// NOWMO-NOT: #OpenA.yesOverrides{{.*}} [nonoverridden]
// NOWMO: #OpenA.isFinal{{.*}} [nonoverridden]
// NOWMO: #OpenA.isFinal{{[^[]]*}}

// WMO-LABEL: sil_vtable OpenA {
// WMO-NOT: #OpenA.noOverrides{{.*}} [nonoverridden]
Expand All @@ -175,7 +175,7 @@ sil @OpenB_yesOverrides : $@convention(method) (@guaranteed OpenB) -> ()
// NOWMO-LABEL: sil_vtable OpenB {
// NOWMO-NOT: #OpenA.noOverrides{{.*}} [nonoverridden]
// NOWMO-NOT: #OpenA.yesOverrides{{.*}} [nonoverridden]
// NOWMO: #OpenA.isFinal{{.*}} [nonoverridden]
// NOWMO: #OpenA.isFinal{{[^[]]*}}

// WMO-LABEL: sil_vtable OpenB {
// WMO-NOT: #OpenA.noOverrides{{.*}} [nonoverridden]
Expand Down