Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

missed optimization for inline virtual methods #4133

Open Quuxplusone opened 14 years ago

Quuxplusone commented 14 years ago
Bugzilla Link PR6747
Status NEW
Importance P normal
Reported by Rafael Ávila de Espíndola (rafael@espindo.la)
Reported on 2010-03-30 22:57:02 -0700
Last modified on 2018-03-19 16:01:01 -0700
Version unspecified
Hardware PC Linux
CC dgregor@apple.com, llvm-bugs@lists.llvm.org
Fixed by commit(s)
Attachments PR6747.patch (1257 bytes, text/plain)
PR6747.patch (1988 bytes, text/plain)
PR6747.patch (1992 bytes, text/plain)
Blocks
Blocked by
See also
Given the translation unit

struct foo {
  virtual void bar();
  virtual void baz() {}
};
void zed() {
  foo b;
  b.baz();
}

we currently produce

define linkonce_odr void @_ZN3foo3bazEv

it should probably be available_externally. Given that the function is virtual
and we know we will have a vtable in some other translation unit, we can
produce this one as available_externally.
Quuxplusone commented 14 years ago

Attached PR6747.patch (1257 bytes, text/plain): proposed patch

Quuxplusone commented 14 years ago

Attached PR6747.patch (1988 bytes, text/plain): updated patch

Quuxplusone commented 14 years ago

Attached PR6747.patch (1992 bytes, text/plain): rebased patch

Quuxplusone commented 14 years ago
(In reply to comment #3)
> Created an attachment (id=4663) [details]
> rebased patch

--- a/lib/CodeGen/CodeGenModule.cpp
+++ b/lib/CodeGen/CodeGenModule.cpp
@@ -314,7 +314,15 @@ GetLinkageForFunction(ASTContext &Context, const
FunctionDecl *FD,
   if (FD->getTemplateSpecializationKind()
                                        == TSK_ExplicitInstantiationDeclaration)
     return CodeGenModule::GVA_C99Inline;
-
+
+  if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(FD)) {
+    const CXXRecordDecl *RD = MD->getParent();
+    if (RD->isDynamicClass() &&
+   MD->isVirtual() &&
+   CodeGenVTables::isKeyFunctionInAnotherTU(Context, RD))
+      return CodeGenModule::GVA_C99Inline;
+  }

The RD->isDynamicClass() check is redundant.

Otherwise, this patch looks good. Thanks!
Quuxplusone commented 14 years ago

Fixed in 101757.

Quuxplusone commented 14 years ago

We've had to remove this optimization in r103741; see the comment in that commit. We can revisit this optimization later.