[PATCH] D22296: CodeGen: New vtable group representation: struct of vtable arrays.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

This seems reasonable to me.  Just a few representational / API requests.



================
Comment at: clang/include/clang/AST/VTableBuilder.h:222
+  typedef llvm::DenseMap<BaseSubobject, std::pair<unsigned, unsigned>>
+      AddressPointsMapTy;
 
----------------
Please use a struct instead of std::pair.


================
Comment at: clang/include/clang/AST/VTableBuilder.h:225
 private:
+  std::vector<size_t> VTableIndices;
+
----------------
Does this actually grow, or should you use a representation more like VTableComponents?  Or something custom that exploits the fact that, in the vast majority of cases, this vector will have size 1?


================
Comment at: clang/include/clang/AST/VTableBuilder.h:243
+               uint64_t NumVTableThunks, const VTableThunkTy *VTableThunks,
+               const AddressPointsMapTy &AddressPoints, bool IsMicrosoftABI);
   ~VTableLayout();
----------------
Since you're changing this interface anyway, would you mind fixing it to use ArrayRef for VTableComponents and VTableThunks, too?


================
Comment at: clang/include/clang/AST/VTableBuilder.h:260
+    assert(AddressPoint.second != 0 || IsMicrosoftABI);
     (void)IsMicrosoftABI;
 
----------------
I know you didn't add this line, but could you remove this cast?  IsMicrosoftABI is not a local variable.

Also, the assert above suggests that this can be:
  ... = AddressPoint.find(Base)->second;
which should be slightly more efficient than lookup().  Also, I think the assertion becomes unnecessary at that point.


================
Comment at: clang/lib/AST/VTableBuilder.cpp:986
+  /// vtable group.
+  std::vector<size_t> VTableIndices;
+
----------------
Assuming you stop storing a std::vector in the VTableLayout, you should make this a SmallVector.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:644
+
+  std::vector<llvm::Type *> tys;
+  for (unsigned i = 0, e = idxs.size(); i != e; ++i) {
----------------
SmallVector, please.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:648
+    size_t nextIndex =
+        (i + 1 == e) ? layout.vtable_components().size() : idxs[i + 1];
+    tys.push_back(llvm::ArrayType::get(CGM.Int8PtrTy, nextIndex - thisIndex));
----------------
This seems like a completely reasonable query to add to VTableLayout:
  unsigned getVTableSize(unsigned vtableIndex) const;
or maybe:
  ArrayRef<VTableComponent> getVTableComponents(unsigned vtableIndex) const;

I guess you'll also need a getNumVTables().


https://reviews.llvm.org/D22296