[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:235
+
+void HierarchyBuilder::addFragment(Metadata *Type,
+                                   const std::set<GlobalVariable *> &GlobalSet,
----------------
I think this function can still be simplified. For example, do you still need the `Fragments` array? It seems like you only need to keep track of whether each `GlobalVariable` has been seen before and which `Metadata` it was last seen with.


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:243
+
+  for (auto GV : GlobalSet) {
+    if (FragmentMap.find(GV) == FragmentMap.end()) {
----------------
This will end up adding the globals in an order determined by their pointer values, which I believe will make the order of the elements added to `ExclusiveVTables` non-deterministic. As I mentioned in another comment I think this should take a vector (or, perhaps better, an ArrayRef).


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:256
+      Metadata *ChildType = TypeMap[OldFragmentIndex];
+      if (TII.ChildrenTypes.find(ChildType) == TII.ChildrenTypes.end())
+        TII.ChildrenTypes.emplace(ChildType);
----------------
You can just insert the new type, the set will take care of deduplication. Since this will be another source of non-determinism, I'd suggest using a `SetVector` instead of a `std::set`.


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:616
+  // Set the start index of the vtable in the list for the type.
+  InterleavingInfoMap[BaseType].StartIndex = OrderedGlobals.size();
+
----------------
Maybe do the lookup once:
```
TypeInterleavingInfo &Info = InterleavingInfoMap[BaseType];
```
and use `Info` in the rest of the code?


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:664
+  // the type identifier.
+  std::map<Metadata *, std::set<GlobalVariable *>> TypeMembersMap;
+
----------------
I think that for determinism this needs to be a `MapVector` and the set needs to be a vector. I think you could initialize it here by adding empty values for each element of `TypeIds` here (after following my other suggestion to pass that instead of `TypeAndUniqueIds`).


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:670
+      Metadata *TypeId = Type->getOperand(1);
+      if (TypeAndUniqueIds.find(TypeId) != TypeAndUniqueIds.end())
+        TypeMembersMap[TypeId].insert(GTM->getGlobal());
----------------
With my above suggestion this can now be a lookup on `TypeMembersMap`.


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:712
+  // Sort entry groups in the order of decreasing size
+  llvm::sort(Groups.begin(), Groups.end(),
+             [&](const EntryGroup &Left, const EntryGroup &Right) {
----------------
Could this be a `std::stable_sort` on the size alone?


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:785
+        report_fatal_error(
+            "All operands of offset.type metadata must have 1 element");
+      int64_t Offset;
----------------
What do you think about putting the offset in the metadata as an additional operand?


================
Comment at: llvm/lib/Transforms/IPO/InterleaveVTables.cpp:934
+    for (auto TypeId : TypeIds)
+      TypeAndUniqueIds[TypeId] = TypeIdInfo[TypeId].UniqueId;
+
----------------
Is it necessary to create this map? It looks like `handleDisjointSet` never uses the values so you can just pass in `TypeIds`.


================
Comment at: llvm/test/Transforms/InterleaveVTables/basic.ll:15
+
+ at __typeid1$0 = constant i64 0, !offset.type !3
+ at __typeid1$1 = constant i64 1, !offset.type !3
----------------
Shouldn't there be a part of this test (and others) that uses these variables so that you can check that they've been replaced with the correct values?


https://reviews.llvm.org/D50754