[PATCH] D29811: WholeProgramDevirt: Implement exporting for single-impl devirtualization.
Peter Collingbourne via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 3 16:38:56 PST 2017
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
More information about the llvm-commits
mailing list