[PATCH] D50754: Implementation of a vtable interleaving algorithm

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

Please make sure that your patch is formatted to match the LLVM style. `clang-format` can do that for you.

A couple of other global style comments that `clang-format` won't take care of for you:

- instead of writing `(foo.bar)->baz`, please write `foo.bar->baz`.
- no braces around the body of an `if`, `for` etc. statement if it consists of a single statement. That includes nested statements which should be rewritten as

  if (foo)
    for (auto bar : baz)
      f(bar);



================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:284
+    for (Metadata *TypeId : TypeIds) {
+      auto TypeIdStr = dyn_cast<MDString>(TypeId);
+      int Offset = 0;
----------------
There's a problem here that I only just noticed (sorry). Type identifiers are not guaranteed to be `MDString`s -- they can also be distinct `MDNode`s if the type has internal linkage (see http://llvm-cs.pcc.me.uk/tools/clang/lib/CodeGen/CodeGenModule.cpp#4986 for how this is handled in the frontend) -- these `MDNode`s are deliberately not convertible to strings because they need to be scoped to the translation unit. So I think you will need to create a new metadata kind here: http://llvm-cs.pcc.me.uk/include/llvm/IR/LLVMContext.h#99 and attach it to the global variables that you create in order to provide the mapping from global variables to metadata.


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:289
+
+      if (dyn_cast<ConstantInt>(GV.getInitializer()) == nullptr)
+        report_fatal_error("The offset global variable is not a ConstantInt.");
----------------
I don't think we should expect there to be any particular type of initializer on these variables; the initializer that they're created with in the frontend is arbitrary because nobody should be reading from them.


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:357
+/// return a constant element in this array at Offset
+Constant * InterleaveVTablesModule::getConstantFromArray(Constant * Array, unsigned Offset) {
+
----------------
I think you can replace this function with a call to `getAggregateElement`: http://llvm-cs.pcc.me.uk/lib/IR/Constants.cpp#328


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:408
+  // them.
+  std::vector<std::tuple<Metadata *, int64_t, GlobalVariable *>> SameDistanceEntries;
+
----------------
I'd declare a struct type for this data structure instead of using `std::tuple` so that you can give an appropriate name to each field.


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:475
+
+//#ifdef EXPENSIVE_CHECKS
+  // Verify that each entry in a old VTable has the same content as the corresponding
----------------
Why is this commented out?


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:586
+
+  if (Globals.size() == 0)
+    return;
----------------
I don't think you need this line because you should always expect there to be at least one global in the set.


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:609
+
+  // Order the sets of indices by size. The GlobalLayoutBuilder works best
+  // when given small index sets first.
----------------
The comment should talk about how this is a requirement in order to group the vtables correctly.


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:722
+    for (MDNode *Type : Types) {
+      uint64_t Offset = verifyTypeMDNode(&GV, Type);
+      if (BytesBeforeAddrPt > Offset)
----------------
I'd inline the `verifyTypeMDNode` function here because the function's name doesn't correspond to its return value.


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:723
+      uint64_t Offset = verifyTypeMDNode(&GV, Type);
+      if (BytesBeforeAddrPt > Offset)
+    	BytesBeforeAddrPt = Offset;
----------------
This should assert that all offsets are the same. We shouldn't expect to see different offsets here except for with the metadata associated with virtual member function pointers. And on the frontend side we should disable member function pointer metadata when interleaving is enabled since there would need to be a number of other things fixed before we could turn that on.


================
Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:155
 
+static cl::opt<bool> InterleaveOrNot("interleave", cl::desc("Enable VTables interleaving"));
+
----------------
I'd call this something like `-enable-vtable-interleaving`. The name of the variable should also match the name of the flag like the other variables.


================
Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:965
   // link time if CFI is enabled. The pass does nothing if CFI is disabled.
   PM.add(createLowerTypeTestsPass(ExportSummary, nullptr));
 
----------------
I'd expect this line to be in an else clause after your if statement above.


https://reviews.llvm.org/D50754