[PATCH] D11859: Generating vptr assume loads

[David Majnemer via cfe-commits]

majnemer added a comment.

Some first round review comments.


================
Comment at: lib/CodeGen/CGClass.cpp:1831-1832
@@ +1830,4 @@
+
+  // generate vtable assumptions if we are not in another ctor and
+  // if we calling dynamic class ctor
+  if (CGM.getCodeGenOpts().OptimizationLevel > 0 &&
----------------
Sentences should start with a capital letter and end with a period.

================
Comment at: lib/CodeGen/CGClass.cpp:1838
@@ +1837,3 @@
+
+void CodeGenFunction::EmitVTableAssumptionLoad(const VPtr &vptr,
+                                               llvm::Value *This) {
----------------
Variables should be capitalized.

================
Comment at: lib/CodeGen/CGClass.cpp:1842
@@ +1841,3 @@
+      CGM.getCXXABI().getVTableAddressPoint(vptr.Base, vptr.VTableClass);
+  if (!VTableGlobal) return;
+
----------------
The return should be on it's own line, please clang-format this.
Also, under which conditions is this case possible?

================
Comment at: lib/CodeGen/CGClass.cpp:2098
@@ -2063,3 +2097,3 @@
     // Initialize the vtable pointer for this base.
-    InitializeVTablePointer(Base, NearestVBase, OffsetFromNearestVBase,
-                            VTableClass);
+    VPtr vptr{Base, NearestVBase, OffsetFromNearestVBase, VTableClass};
+    vptrs.push_back(vptr);
----------------
This kind of initialization is not very common in LLVM.

================
Comment at: lib/CodeGen/CodeGenFunction.h:1312
@@ -1311,1 +1311,3 @@
 
+  // struct containg all informations needed to initilize vptrs
+  struct VPtr {
----------------
Capital letter and period missing here.

================
Comment at: lib/CodeGen/CodeGenFunction.h:1328-1334
@@ -1320,8 +1327,9 @@
   typedef llvm::SmallPtrSet<const CXXRecordDecl *, 4> VisitedVirtualBasesSetTy;
-  void InitializeVTablePointers(BaseSubobject Base,
-                                const CXXRecordDecl *NearestVBase,
-                                CharUnits OffsetFromNearestVBase,
-                                bool BaseIsNonVirtualPrimaryBase,
-                                const CXXRecordDecl *VTableClass,
-                                VisitedVirtualBasesSetTy& VBases);
+  VPtrsVector getVTablePointers(const CXXRecordDecl *VTableClass);
+
+  void getVTablePointers(BaseSubobject Base, const CXXRecordDecl *NearestVBase,
+                         CharUnits OffsetFromNearestVBase,
+                         bool BaseIsNonVirtualPrimaryBase,
+                         const CXXRecordDecl *VTableClass,
+                         VisitedVirtualBasesSetTy &VBases, VPtrsVector &vptrs);
 
----------------
Would it make more sense for these to live in `CGClass` ?

================
Comment at: lib/CodeGen/ItaniumCXXABI.cpp:193-194
@@ -192,1 +192,4 @@
 
+  bool isVirtualOffsetNeeded(CodeGenFunction &CGF,
+                             const CXXRecordDecl *NearestVBase) override;
+
----------------
I'm not sure what this function is supposed to compute.  Needed for what?

================
Comment at: lib/CodeGen/ItaniumCXXABI.cpp:1404
@@ +1403,3 @@
+    const CXXRecordDecl *NearestVBase) {
+  // TODO check it
+  if ((Base.getBase()->getNumVBases() || NearestVBase != nullptr) &&
----------------
Check what?

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:223-228
@@ +222,8 @@
+  // with the 'novtable' attribute.
+  bool canInitilizeVPtr(const CXXRecordDecl *VTableClass,
+                        const CXXRecordDecl *Base,
+                        const CXXRecordDecl *NearestVBase) override {
+    return !VTableClass->hasAttr<MSNoVTableAttr>() ||
+           (Base != VTableClass && Base != NearestVBase);
+  }
+
----------------
In the MS ABI, derived classes never share vtables with bases.  Why do you need to do anything other than check that the most derived class doesn't have the `__declspec(novtable)` ?


http://reviews.llvm.org/D11859