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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:290
   std::vector<VirtualCallSite> CallSites;
+  bool HasTypeTestAssumeUsers;
   std::vector<FunctionSummary *> TypeCheckedLoadUsers;
----------------
tejohnson wrote:
> 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.
Both of the members I'm adding in this patch are ThinLTO-specific. I've added a comment here and an outline to the top of the file in D29808.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:609
+    if (CSInfo.isExported()) {
+      IsExported = true;
+      CSInfo.TypeCheckedLoadUsers.clear();
----------------
tejohnson wrote:
> Suggest setting IsExported to false if not, so that it will always be set by this routine.
We need to leave this field as it is if `CSInfo.isExported()` is false, because a previous call to this lambda may have set it to true and we need to remember that.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:610
+      IsExported = true;
+      CSInfo.TypeCheckedLoadUsers.clear();
+    }
----------------
tejohnson wrote:
> Needs comment for this block or at least on why this is being cleared.
Added to the definition of CallSiteInfo in D29808


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1118
+      WholeProgramDevirtResolution *Res = nullptr;
+      if (Action == PassSummaryAction::Export && isa<MDString>(S.first.TypeID))
+        Res =
----------------
tejohnson wrote:
> What does it mean for the TypeID to not be an MDString?
It generally means that the type ID is a distinct MDNode and is therefore local to the module. This can happen if the merged module contains a module compiled with regular LTO or a module for which we could not generate a module ID (so we weren't able to turn it into an MDString by exporting it -- see http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp#114).

Added test coverage for this case.


https://reviews.llvm.org/D29811