[PATCH] D29844: WholeProgramDevirt: Implement importing for single-impl devirtualization.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:404
 
+  void importResolution(VTableSlot Slot, VTableSlotInfo &SlotInfo);
+
----------------
Document new functions.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1052
+  const WholeProgramDevirtResolution &Res =
+      Summary->getTypeIdSummary(cast<MDString>(Slot.TypeID)->getString())
+          .WPDRes[Slot.ByteOffset];
----------------
On the export side there were checks on whether TypeID was an MDString, needed here too?


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1057
+    auto *SingleImpl = M.getOrInsertFunction(
+        Res.SingleImplName, Type::getVoidTy(M.getContext()), nullptr);
+    bool IsExported;
----------------
I forget - is there a constraint that the single implementation function return void and not take any args?


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1059
+    bool IsExported;
+    applySingleImplDevirt(SlotInfo, SingleImpl, IsExported);
+  }
----------------
I assume IsExported should never be true since we invoke this on import? Should that be checked?


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1097
+    removeUnusedTypeTests();
+    return true;
+  }
----------------
Why can we skip the rest of the function in import mode? E.g. we won't get an optimization remark emitted. Or is the rest of the code only necessary on the export side? Needs a comment at least.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1162
   // eliminate the type test by replacing it with true.
-  if (TypeCheckedLoadFunc) {
-    auto True = ConstantInt::getTrue(M.getContext());
----------------
This guard went away - why?


https://reviews.llvm.org/D29844