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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc marked 2 inline comments as done.
pcc added inline comments.


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


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1057
+    auto *SingleImpl = M.getOrInsertFunction(
+        Res.SingleImplName, Type::getVoidTy(M.getContext()), nullptr);
+    bool IsExported;
----------------
tejohnson wrote:
> I forget - is there a constraint that the single implementation function return void and not take any args?
The type of the function in the declaration is irrelevant because every call site will cast it to the correct type. Added comment.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1097
+    removeUnusedTypeTests();
+    return true;
+  }
----------------
tejohnson wrote:
> 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.
Yes, the rest of the code is only necessary on the export side or during regular LTO (the remarks as well -- the export phase will emit remarks for the whole program).


================
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());
----------------
tejohnson wrote:
> This guard went away - why?
It is unnecessary. The map `NumUnsafeUsesForTypeTest` will be empty if `TypeCheckedLoadFunc` is null.


https://reviews.llvm.org/D29844