[PATCH] D29811: WholeProgramDevirt: Implement exporting for single-impl devirtualization.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:290
   std::vector<VirtualCallSite> CallSites;
+  bool HasTypeTestAssumeUsers;
   std::vector<FunctionSummary *> TypeCheckedLoadUsers;
----------------
How many of these fields are specific to ThinLTO? I assume at least the isExported method is ThinLTO-specific? It would be good to document that.

Also, as I'm reviewing these patches I'm getting confused as to what aspects of the implementation of the optimization are for ThinLTO, it would be good to include a rough outline of how this works for ThinLTO in this file.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:609
+    if (CSInfo.isExported()) {
+      IsExported = true;
+      CSInfo.TypeCheckedLoadUsers.clear();
----------------
Suggest setting IsExported to false if not, so that it will always be set by this routine.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:610
+      IsExported = true;
+      CSInfo.TypeCheckedLoadUsers.clear();
+    }
----------------
Needs comment for this block or at least on why this is being cleared.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:637
+
+  // If the only implementation has local linkage, we must promote to external
+  // to make it visible to thin LTO objects.
----------------
Do we only ever have IsExported==true and therefore reach here in the ThinLTO case? Please add note to that effect if so.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1118
+      WholeProgramDevirtResolution *Res = nullptr;
+      if (Action == PassSummaryAction::Export && isa<MDString>(S.first.TypeID))
+        Res =
----------------
What does it mean for the TypeID to not be an MDString?


https://reviews.llvm.org/D29811