[PATCH] D50754: Implementation of a vtable interleaving algorithm

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:337
+
+  // Initialize UpperEntires and LowerEntries.
+  for (auto GV : OrderedGlobals) {
----------------
Nit: UpperEntries


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:359
+  int64_t InterleavedIndex;
+  for (const auto &Group : Groups) {
+    Metadata *TypeId = Group.TypeId;
----------------
Please write the type here instead of `auto`. We generally write out the type unless it's obvious what it is.


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:416
+  // corresponding entry in the interleaved table does.
+  for (const auto &GV : OrderedGlobals) {
+    ConstantArray *OldArray = dyn_cast<ConstantArray>(GV->getInitializer());
----------------
Should this be in an `#ifdef ENABLE_EXPENSIVE_CHECKS` block?


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:439-464
+    } else { // The initialzer of the global variable is a ConstantDataArray
+      ConstantDataArray *OldDataArray =
+          dyn_cast<ConstantDataArray>(GV->getInitializer());
+      for (int64_t I = 0; I < OldDataArray->getNumElements(); ++I) {
+        if (OldToNewOffsets[GV].count(I - (int64_t)AddressPointIndices[GV]) ==
+            0)
+          continue;
----------------
Can the block of code that handles `ConstantDataArray` be removed if you use `getAggregateElement` above?


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:470
+  for (auto &Elem : InterleavedEntries)
+    if (Elem->getType() != Int8PtrTy)
+      Elem = ConstantExpr::getIntToPtr(Elem, Int8PtrTy);
----------------
I think this won't correctly handle pointers that are not of type `i8*` (because `inttoptr` doesn't work on pointers). This should use a bitcast if the operand is a pointer.


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:529
+    std::map<int64_t, GlobalVariable *> ProcessedGroups) {
+  auto &TII = InterleavingInfoMap[BaseType];
+
----------------
Please write the type here.


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:537
+  // appear consecutively.
+  std::vector<GlobalVariable *> &ExclusiveVTables = TII.ExclusiveVTables;
+  OrderedGlobals.insert(OrderedGlobals.end(), ExclusiveVTables.begin(),
----------------
You don't need this variable, you can just write `TII.ExclusiveVTables` everywhere.


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:545-546
+  // that are not in ProcessedGroups.
+  std::vector<int64_t> &Offsets = TII.Offsets;
+  std::vector<GlobalVariable *> &OffsetGlobals = TII.OffsetGlobals;
+  for (size_t I = 0; I < Offsets.size(); I++) {
----------------
Same for these.


https://reviews.llvm.org/D50754