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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

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

rL370597 


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66621/new/

https://reviews.llvm.org/D66621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370597: [clang] Devirtualization for classes with 
destructors marked as final (authored by xbolva00, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66621?vs=217009=218230#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66621/new/

https://reviews.llvm.org/D66621

Files:
  cfe/trunk/lib/AST/DeclCXX.cpp
  cfe/trunk/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp


Index: cfe/trunk/lib/AST/DeclCXX.cpp
===
--- cfe/trunk/lib/AST/DeclCXX.cpp
+++ cfe/trunk/lib/AST/DeclCXX.cpp
@@ -2067,10 +2067,15 @@
   if (DevirtualizedMethod->hasAttr())
 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())
 return DevirtualizedMethod;
+  if (const auto *dtor = BestDynamicDecl->getDestructor()) {
+if (dtor->hasAttr())
+  return DevirtualizedMethod;
+  }
 
   if (const auto *DRE = dyn_cast(Base)) {
 if (const auto *VD = dyn_cast(DRE->getDecl()))
Index: cfe/trunk/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
===
--- cfe/trunk/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
+++ cfe/trunk/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
@@ -24,11 +24,24 @@
   }
 }
 
-namespace Test3 {
+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


Index: cfe/trunk/lib/AST/DeclCXX.cpp
===
--- cfe/trunk/lib/AST/DeclCXX.cpp
+++ cfe/trunk/lib/AST/DeclCXX.cpp
@@ -2067,10 +2067,15 @@
   if (DevirtualizedMethod->hasAttr())
 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())
 return DevirtualizedMethod;
+  if (const auto *dtor = BestDynamicDecl->getDestructor()) {
+if (dtor->hasAttr())
+  return DevirtualizedMethod;
+  }
 
   if (const auto *DRE = dyn_cast(Base)) {
 if (const auto *VD = dyn_cast(DRE->getDecl()))
Index: cfe/trunk/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
===
--- cfe/trunk/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
+++ cfe/trunk/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
@@ -24,11 +24,24 @@
   }
 }
 
-namespace Test3 {
+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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Logan Smith via Phabricator via cfe-commits]

logan-5 added a comment.

//ping//


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66621/new/

https://reviews.llvm.org/D66621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Logan Smith via Phabricator via cfe-commits]

logan-5 updated this revision to Diff 217009.
logan-5 added a comment.

Add a missing null check.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66621/new/

https://reviews.llvm.org/D66621

Files:
  clang/lib/AST/DeclCXX.cpp
  clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp


Index: clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
===
--- clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
+++ clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
@@ -24,6 +24,20 @@
   }
 }
 
+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();
Index: clang/lib/AST/DeclCXX.cpp
===
--- clang/lib/AST/DeclCXX.cpp
+++ clang/lib/AST/DeclCXX.cpp
@@ -2067,10 +2067,15 @@
   if (DevirtualizedMethod->hasAttr())
 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())
 return DevirtualizedMethod;
+  if (const auto *dtor = BestDynamicDecl->getDestructor()) {
+if (dtor->hasAttr())
+  return DevirtualizedMethod;
+  }
 
   if (const auto *DRE = dyn_cast(Base)) {
 if (const auto *VD = dyn_cast(DRE->getDecl()))


Index: clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
===
--- clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
+++ clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
@@ -24,6 +24,20 @@
   }
 }
 
+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();
Index: clang/lib/AST/DeclCXX.cpp
===
--- clang/lib/AST/DeclCXX.cpp
+++ clang/lib/AST/DeclCXX.cpp
@@ -2067,10 +2067,15 @@
   if (DevirtualizedMethod->hasAttr())
 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())
 return DevirtualizedMethod;
+  if (const auto *dtor = BestDynamicDecl->getDestructor()) {
+if (dtor->hasAttr())
+  return DevirtualizedMethod;
+  }
 
   if (const auto *DRE = dyn_cast(Base)) {
 if (const auto *VD = dyn_cast(DRE->getDecl()))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hiroshi Yamauchi via Phabricator via cfe-commits]

yamauchi added a comment.

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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66621/new/

https://reviews.llvm.org/D66621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Logan Smith via Phabricator via cfe-commits]

logan-5 marked an inline comment as done.
logan-5 added a comment.

@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!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66621/new/

https://reviews.llvm.org/D66621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Logan Smith via Phabricator via cfe-commits]

logan-5 updated this revision to Diff 216757.
logan-5 added a comment.

Tweaked comment wording for accuracy.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66621/new/

https://reviews.llvm.org/D66621

Files:
  clang/lib/AST/DeclCXX.cpp
  clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp


Index: clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
===
--- clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
+++ clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
@@ -24,6 +24,20 @@
   }
 }
 
+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();
Index: clang/lib/AST/DeclCXX.cpp
===
--- clang/lib/AST/DeclCXX.cpp
+++ clang/lib/AST/DeclCXX.cpp
@@ -2067,9 +2067,11 @@
   if (DevirtualizedMethod->hasAttr())
 return DevirtualizedMethod;
 
-  // Similarly, if the class itself is marked 'final' it can't be overridden
-  // and we can therefore devirtualize the member function call.
-  if (BestDynamicDecl->hasAttr())
+  // 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() ||
+  BestDynamicDecl->getDestructor()->hasAttr())
 return DevirtualizedMethod;
 
   if (const auto *DRE = dyn_cast(Base)) {


Index: clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
===
--- clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
+++ clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
@@ -24,6 +24,20 @@
   }
 }
 
+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();
Index: clang/lib/AST/DeclCXX.cpp
===
--- clang/lib/AST/DeclCXX.cpp
+++ clang/lib/AST/DeclCXX.cpp
@@ -2067,9 +2067,11 @@
   if (DevirtualizedMethod->hasAttr())
 return DevirtualizedMethod;
 
-  // Similarly, if the class itself is marked 'final' it can't be overridden
-  // and we can therefore devirtualize the member function call.
-  if (BestDynamicDecl->hasAttr())
+  // 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() ||
+  BestDynamicDecl->getDestructor()->hasAttr())
 return DevirtualizedMethod;
 
   if (const auto *DRE = dyn_cast(Base)) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

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).




Comment at: clang/lib/AST/DeclCXX.cpp:2071
+  // Similarly, if the class itself or its destructor is marked 'final',
+  // the class can't be overridden and we can therefore devirtualize the 
+  // member function call.

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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66621/new/

https://reviews.llvm.org/D66621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Aha okay :)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66621/new/

https://reviews.llvm.org/D66621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Logan Smith via Phabricator via cfe-commits]

logan-5 added a comment.

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`.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66621/new/

https://reviews.llvm.org/D66621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

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


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66621/new/

https://reviews.llvm.org/D66621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Logan Smith via Phabricator via cfe-commits]

logan-5 created this revision.
logan-5 added a reviewer: rsmith.
logan-5 added a project: clang.
Herald added subscribers: cfe-commits, Prazek.

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`.


Repository:
  rC Clang

https://reviews.llvm.org/D66621

Files:
  clang/lib/AST/DeclCXX.cpp
  clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp


Index: clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
===
--- clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
+++ clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
@@ -24,6 +24,20 @@
   }
 }
 
+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();
Index: clang/lib/AST/DeclCXX.cpp
===
--- clang/lib/AST/DeclCXX.cpp
+++ clang/lib/AST/DeclCXX.cpp
@@ -2067,9 +2067,11 @@
   if (DevirtualizedMethod->hasAttr())
 return DevirtualizedMethod;
 
-  // Similarly, if the class itself is marked 'final' it can't be overridden
-  // and we can therefore devirtualize the member function call.
-  if (BestDynamicDecl->hasAttr())
+  // Similarly, if the class itself or its destructor is marked 'final',
+  // the class can't be overridden and we can therefore devirtualize the 
+  // member function call.
+  if (BestDynamicDecl->hasAttr() ||
+  BestDynamicDecl->getDestructor()->hasAttr())
 return DevirtualizedMethod;
 
   if (const auto *DRE = dyn_cast(Base)) {


Index: clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
===
--- clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
+++ clang/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
@@ -24,6 +24,20 @@
   }
 }
 
+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();
Index: clang/lib/AST/DeclCXX.cpp
===
--- clang/lib/AST/DeclCXX.cpp
+++ clang/lib/AST/DeclCXX.cpp
@@ -2067,9 +2067,11 @@
   if (DevirtualizedMethod->hasAttr())
 return DevirtualizedMethod;
 
-  // Similarly, if the class itself is marked 'final' it can't be overridden
-  // and we can therefore devirtualize the member function call.
-  if (BestDynamicDecl->hasAttr())
+  // Similarly, if the class itself or its destructor is marked 'final',
+  // the class can't be overridden and we can therefore devirtualize the 
+  // member function call.
+  if (BestDynamicDecl->hasAttr() ||
+  BestDynamicDecl->getDestructor()->hasAttr())
 return DevirtualizedMethod;
 
   if (const auto *DRE = dyn_cast(Base)) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits