[clang] Devirtualization for classes with destructors marked as 'final'

[logan-5]

Summary

A class with a destructor marked final cannot be derived from, so it should afford the same devirtualization opportunities as marking the entire class final.

[xbolva00]

https://reviews.llvm.org/rGcb30590da10ba80a133222588efedab7e5fc9bdd does not help here?

[logan-5]

That appears sort of tangential to me. To clarify, this PR is not (necessarily) about devirtualizing destructors themselves, but rather devirtualizing OTHER member function calls for classes whose destructor is marked final, since that is sort of morally equivalent to marking the entire class final.

[xbolva00]

Aha okay :)

[rsmith]

This seems subtle, but I believe it is correct.

I wonder whether we should provide a warning for a non-final class has a final destructor, since moving the final from the destructor to the class seems like a more obvious way to present the code (and will likely lead to better code generation in compilers that haven't realized they can do this).

clang/lib/AST/DeclCXX.cpp

2074

This comment doesn't make much sense (pre-existing, but since you're changing it anyway): classes can't be "overridden", they can only be derived from.

[logan-5]

Tweaked comment wording for accuracy.

[logan-5]

@rsmith I agree having a final destructor in a non-final class smells weird. I'd be interested in working on adding a warning like that, if it sounds like a useful thing.

For now, I need help committing this, if anyone would be so kind!

[hjyamauchi]

Nice.

In D66621#1642142, @rsmith wrote:

This seems subtle, but I believe it is correct.

I wonder whether we should provide a warning for a non-final class has a final destructor, since moving the final from the destructor to the class seems like a more obvious way to present the code (and will likely lead to better code generation in compilers that haven't realized they can do this).

Richard, do you think there may be some missed devirtualization optimizations that would trigger if the class, rather than the destructor, is declared final in Clang? It seems this patch would take care of common call devirtualizations.

[logan-5]

Add a missing null check.

[logan-5]

ping

[xbolva00]

For now, I need help committing this, if anyone would be so kind!

rL370597

clang/lib/AST/DeclCXX.cpp

CXXMethodDecl *CXXMethodDecl::getDevirtualizedMethod(const Expr *Base,
   // function, and we can't assume the derived class function is defined.
   if (DevirtualizedMethod->isPure())
     return nullptr;
 
   // If that method is marked final, we can devirtualize it.
   if (DevirtualizedMethod->hasAttr<FinalAttr>())
     return DevirtualizedMethod;
 
-  // Similarly, if the class itself is marked 'final' it can't be overridden
-  // and we can therefore devirtualize the member function call.
+  // Similarly, if the class itself or its destructor is marked 'final',
+  // the class can't be derived from and we can therefore devirtualize the 
+  // member function call.
   if (BestDynamicDecl->hasAttr<FinalAttr>())
     return DevirtualizedMethod;
+  if (const auto *dtor = BestDynamicDecl->getDestructor()) {
+    if (dtor->hasAttr<FinalAttr>())
+      return DevirtualizedMethod;
+  }
 
   if (const auto *DRE = dyn_cast<DeclRefExpr>(Base)) {
     if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl()))
       if (VD->getType()->isRecordType())
         // This is a record decl. We know the type and can devirtualize it.
         return DevirtualizedMethod;
 
     return nullptr;

clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp

namespace Test2 {
 
   // CHECK-LABEL: define i32 @_ZN5Test21fEPNS_1AE
   int f(A *a) {
     // CHECK: call i32 @_ZN5Test21A1fEv
     return a->f();
   }
 }
 
+namespace Test2a {
+  struct A {
+    virtual ~A() final {}
+    virtual int f();
+  };
+
+  // CHECK-LABEL: define i32 @_ZN6Test2a1fEPNS_1AE
+  int f(A *a) {
+    // CHECK: call i32 @_ZN6Test2a1A1fEv
+    return a->f();
+  }
+}
+
+
 namespace Test3 {
   struct A {
     virtual int f();
   };
 
   struct B final : A { };
 
   // CHECK-LABEL: define i32 @_ZN5Test31fEPNS_1BE