[PATCH] D17212: [ThinLTO] Support for call graph in per-module and combined summary.

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2572
@@ +2571,3 @@
+              CallGraphEdges[CalleeId] += ScaledCount;
+              RefEdges.erase(CalleeId);
+            }
----------------
tejohnson wrote:
> joker.eph wrote:
> > tejohnson wrote:
> > > joker.eph wrote:
> > > > You depend on the order of the calls.
> > > > If an instruction first add a call to a function, but later on will reference it, you won't remove it from RefEdges.
> > > > 
> > > > This may be intended and you want to have an accurate count of ref + calls and here you're trying to filter calls out of refedges. However a call instruction could legitimately refer the function (a call passing a function pointer as an argument for instance)
> > > > 
> > > > 
> > > I don't think the order of calls matters? The reference list is populated once outside of this loop.
> > > 
> > > On your second point, that is true that this will cause a function both called and referenced in another way to only be recorded as called by this function. Is it important to list both types of references? I was thinking that it was important for importing needs to distinguish the functions being called from the other non-call references, but that essentially the combination of the two are the full reference set of the function. If I should put it in both places, I will need to figure out how to distinguish the two types of function references in findRefEdges.
> > I pointed what I saw as an inconsistency, but I may misunderstand totally what you're trying to do here.
> > 
> > So I will assume in the following that:
> > 
> > - RefEdges contains the global values referenced by the current function
> > - CallGraphEdges contains the list of Functions called by the current function
> > 
> > Do we agree that we should have either of these properties:
> > 
> > 1) A function that is part of CallGraphEdges should *not* be present in RefEdges even if it is referenced in another way than a call
> > 2) A function that is part of CallGraphEdges should *also* be present in RefEdges *if it is referenced* in another way than a call
> > 
> > What I read from your code right now is:
> > 
> > 3) A function that is part of CallGraphEdges *may or may not* be present in RefEdges *if it is referenced* in another way than a call.
> > 
> Yep, I was distracted by the bogus loop above, we are collecting this per instruction so there is an ordering issue, and we are getting #3 which is undesirable.
> 
> I think the way to fix this is to check for the callsite first, then pass in some info from the callsite to findRefEdges so that the callsite reference is itself ignored. I.e. pass in either the callee GV and skip it to get #1, or pass in the Use to get #2. 
> 
> I saw David's follow-on that he thinks #2 is best. I can try that.
Actually it turns out to be pretty simple, we only need to pass in the callee GV to findRefEdges to exclude it (because you can't have both a call and a non-call reference to a function in the same instruction, at least I couldn't find a way!).

For xalancbmk this fix actually reduced the combined index and .o sizes a bit. It turns out that with the old flow we were inadvertently including intrinsics (which were ignored in the call graph edge list) in the reference list.

In the new version of the patch I will upload shortly I've included a new test that checks for both issues (ensuring a function is in both the call list and ref list if it is accessed both ways by the function, and ignoring intrinsics).


http://reviews.llvm.org/D17212