Avoid unnecessarily storing vtable pointers in more destructor cases

[scott-0]

Summary

This makes an existing optimization apply in more cases.

[rsmith]

lib/CodeGen/CGClass.cpp

1369–1382

This is not complete; you'll also need to at least consider atomic stores (that might store the this pointer and be picked up by another thread), the implicit call to operator new in a CXXNewExpr (which is not modeled as a CallExpr), implicit destructor calls (which aren't modeled in the AST at all), and probably a lot of other cases.

Maybe it would be simpler to store undef to the vptr immediately before calling the base class destructor, and let the LLVM optimization passes remove this vptr store as dead if it can prove the vptr is unused?

[scott-0]

Thanks for the review and advice; I'll give undef a try. It's a much simpler approach.

lib/CodeGen/CGClass.cpp

 #include "CGCXXABI.h"
 #include "CGDebugInfo.h"
 #include "CGRecordLayout.h"
 #include "CodeGenFunction.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/EvaluatedExprVisitor.h"
 #include "clang/AST/RecordLayout.h"
+#include "clang/AST/SideEffectFinder.h"
 #include "clang/AST/StmtCXX.h"
 #include "clang/Basic/TargetBuiltins.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "clang/Frontend/CodeGenOptions.h"
 #include "llvm/IR/Intrinsics.h"
 
 using namespace clang;
 using namespace CodeGen;
FieldHasTrivialDestructorBody(ASTContext &Context,
 
   // The destructor for an implicit anonymous union member is never invoked.
   if (FieldClassDecl->isUnion() && FieldClassDecl->isAnonymousStructOrUnion())
     return false;
 
   return HasTrivialDestructorBody(Context, FieldClassDecl, FieldClassDecl);
 }
 
+namespace {
+  /// \brief Detect if any sub-Expr is possibly sensitive to dynamic types.
+  class DynamicTypeSensitivityDetector :
+      public ConstEvaluatedExprVisitor<DynamicTypeSensitivityDetector>
+  {
+    typedef ConstEvaluatedExprVisitor<DynamicTypeSensitivityDetector> Inherited;
+    bool Sensitive;
+
+  public:
+    explicit DynamicTypeSensitivityDetector(const ASTContext &Context)
+      : Inherited(Context), Sensitive(false) { }
+
+    bool isSensitive() const { return Sensitive; }
+
+    void VisitCallExpr(const Expr *E) {
+      Sensitive = true;
+    }
+    void VisitCXXTypeidExpr(const Expr *E) {
+      Sensitive = true;
+    }
+    void VisitExpr(const Expr *E) {
+      if (E->getStmtClass() == Stmt::CXXTypeidExprClass ||
+          E->getStmtClass() == Stmt::CXXDynamicCastExprClass) {
+        Sensitive = true;
+      }
+      if (!Sensitive)
+        Inherited::VisitExpr(E);
+    }
+  };
+}
+
 /// CanSkipVTablePointerInitialization - Check whether we need to initialize
 /// any vtable pointers before calling this destructor.
 static bool CanSkipVTablePointerInitialization(ASTContext &Context,
                                                const CXXDestructorDecl *Dtor) {
-  if (!Dtor->hasTrivialBody())
+  // We only need the vtable ptr for the body if it is sensitive to the
+  // dynamic type of '*this' and potentially has side-effects.
+  const Stmt *Body = Dtor->getBody();
+  DynamicTypeSensitivityDetector Detector(Context);
+  Detector.Visit(Body);
+  if (Detector.isSensitive()) {
+      SideEffectFinder Finder(Context, true);
+      Finder.Visit(Body);
+      if (Finder.hasSideEffects())
         return false;
+  }
 
   // Check the fields.
   const CXXRecordDecl *ClassDecl = Dtor->getParent();
   for (const auto *Field : ClassDecl->fields())
     if (!FieldHasTrivialDestructorBody(Context, Field))
       return false;
 
   return true;

test/CodeGenCXX/destructors.cpp

-// RUN: %clang_cc1 %s -triple x86_64-apple-darwin10 -emit-llvm -o - -mconstructor-aliases -fcxx-exceptions -fexceptions -O1 -disable-llvm-optzns > %t
+// RUN: %clang_cc1 -I%S %s -triple x86_64-apple-darwin10 -emit-llvm -o - -mconstructor-aliases -fcxx-exceptions -fexceptions -O1 -disable-llvm-optzns > %t
+// RUN: FileCheck --check-prefix=CHECK0 --input-file=%t %s
 // RUN: FileCheck --check-prefix=CHECK1 --input-file=%t %s
 // RUN: FileCheck --check-prefix=CHECK2 --input-file=%t %s
 // RUN: FileCheck --check-prefix=CHECK3 --input-file=%t %s
 // RUN: FileCheck --check-prefix=CHECK4 --input-file=%t %s
 // RUN: FileCheck --check-prefix=CHECK5 --input-file=%t %s
 
+#include <typeinfo> // so we can use 'typeid'
+
 struct A {
   int a;
 
   ~A();
 };
 
 // Base with non-trivial destructor
 struct B : A {
   ~B();
 };
 
+// CHECK0-LABEL: @_ZN1BD1Ev = alias void (%struct.B*)* @_ZN1BD2Ev
+// CHECK0-LABEL: @_ZN1CD1Ev = alias void (%struct.C*)* @_ZN1CD2Ev
+// CHECK0-LABEL: @_ZN2TBD1Ev = alias void (%struct.TB*)* @_ZN2TBD2Ev
+// CHECK0-LABEL: @_ZN2TDD1Ev = alias void (%struct.TD*)* @_ZN2TDD2Ev
+
 B::~B() { }
+// CHECK0-LABEL: define void @_ZN1BD2Ev(%struct.B* %this)
+// CHECK0:       call void @_ZN1AD2Ev
 
 // Field with non-trivial destructor
 struct C {
   A a;
 
   ~C();
 };
 
 C::~C() { }
+// CHECK0-LABEL: define void @_ZN1CD2Ev(%struct.C* %this)
+// CHECK0:       call void @_ZN1AD1Ev
+
+struct TB {
+  virtual void f();
+  ~TB();
+  int x;
+};
+TB *tb;
+// None of these statements need the dynamic type to be correct
+// and has side-effects (or are even important to evaluate).
+TB::~TB() { ; {{}} (void)0; ({int y = 42; y; }); x; typeid(this); dynamic_cast<TB*>(tb); }
+// CHECK0-LABEL: define void @_ZN2TBD2Ev(%struct.TB* %this)
+// No need to store vptr.
+// CHECK0-NOT:   @_ZTV
+// CHECK0:  ret
+struct TD : TB {
+  ~TD();
+};
+TD::~TD() { ; {{}} (void)0; ({int y = 42; y; }); x; typeid(this); dynamic_cast<TD*>(tb); }
+// CHECK0-LABEL: define void @_ZN2TDD2Ev(%struct.TD* %this)
+// No need to store vptr.
+// CHECK0-NOT:   @_ZTV
+// CHECK0:  call void @_ZN2TBD2Ev
+// CHECK0:  ret
+
+// Check cases where we must store the vptr.
+extern void g();
+struct NTD : TB {
+  ~NTD();
+};
+NTD::~NTD() { g(); }
+// CHECK0-LABEL: define void @_ZN3NTDD2Ev(%struct.NTD* %this)
+// Because we call 'g()' must to store the vptr (our
+// base doesn't need to, but that's irrelevant).
+// CHECK0:  store {{.*}} @_ZTV3NTD
+// CHECK0:  void @_Z1gv
+// CHECK0:  call void @_ZN2TBD2Ev
+// CHECK0:  ret
+
+struct NTB {
+  virtual void f();
+  ~NTB();
+  int x;
+};
+NTB::~NTB() { g(); }
+// CHECK0-LABEL: define void @_ZN3NTBD2Ev(%struct.NTB* %this)
+// Because we call 'g()' must to store the vptr.
+// CHECK0:  store {{.*}} @_ZTV3NTB
+// CHECK0:  void @_Z1gv
+// CHECK0:  ret
+
+struct TD2 : NTB {
+  virtual void f();
+  ~TD2();
+};
+NTB *ntb;
+TD2::~TD2() { ; {{}} (void)0; ({int y = 42; y; }); x; /*@@@typeid(this); dynamic_cast<TD*>(ntb);*/ }
+// CHECK0-LABEL: define void @_ZN3TD2D2Ev(%struct.TD2* %this)
+// We don't need to store the vptr (our base does need to,
+// but that's irrelevant).
+// CHECK0-NOT:   @_ZTV
+// CHECK0:       call void @_ZN3NTBD2Ev
+// CHECK0:       ret
+
+// More cases we must store the vptr.
+struct NT2 {
+  virtual void f();
+  ~NT2();
+};
+NT2* nt2;
+NT2::~NT2() { nt2->f(); }
+// CHECK0-LABEL: define void @_ZN3NT2D2Ev(%struct.NT2* %this)
+// CHECK0:       store {{.*}} @_ZTV3NT2
+// CHECK0:       ret
+
+struct NT3 {
+  virtual void f();
+  ~NT3();
+};
+const std::type_info *ti;
+NT3::~NT3() { ti = &typeid(this); }
+// CHECK0-LABEL: define void @_ZN3NT3D2Ev(%struct.NT3* %this)
+// CHECK0:       store {{.*}} @_ZTV3NT3
+// CHECK0:       ret
+
+struct NT4 : TB {
+  virtual void f();
+  ~NT4();
+};
+NT4::~NT4() { dynamic_cast<NT4&>(*tb); }
+// CHECK0-LABEL: define void @_ZN3NT4D2Ev(%struct.NT4* %this)
+// CHECK0:       store {{.*}} @_ZTV3NT4
+// CHECK0:       ret
 
 namespace PR7526 {
   extern void foo();
   struct allocator {
     ~allocator() throw();
   };
 
   struct allocator_derived : allocator { };