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

[Xinliang David Li via llvm-commits]

On Tue, Mar 8, 2016 at 11:42 AM, Mehdi AMINI <mehdi.amini at apple.com> wrote:

> joker.eph added inline comments.
>
> ================
> Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:416
> @@ -415,3 +415,3 @@
>  /// files/sections.
>  class FunctionIndexBitcodeReader {
>    DiagnosticHandlerFunction DiagnosticHandler;
> ----------------
> tejohnson wrote:
> > joker.eph wrote:
> > > I guess more renaming could have been done here right?
> > > (I really don't mind, but since you cared to rename many places
> below...)
> > Yep, this was part of the TODO list in this patch update. Working on
> that now. Specifically, I'm changing FunctionIndex* and FunctionInfoIndex*
> to ModuleSummaryIndex*. I left this for a follow-on update because already
> the renaming was pretty extensive, and this particular change bleeds into
> some of the interfaces and variable/field names used in other parts of the
> compiler (clang etc).
> Fine with me.
>
> ================
> Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:852
> @@ -849,2 +851,3 @@
> +    VSTOffsetPlaceholder = WriteValueSymbolTableForwardDecl(Stream);
>    return VSTOffsetPlaceholder;
>  }
> ----------------
> tejohnson wrote:
> > joker.eph wrote:
> > > I'd write it:
> > > ```
> > > if (M->getValueSymbolTable().empty())
> > >   return 0;
> > > return WriteValueSymbolTableForwardDecl(Stream);
> > > ```
> > Good idea, will fix.
> Re-reading it, the variable had the nice property of having a name, making
> it very clear what is returned.
> If you adopt the above suggested change, I would add a one line comment
> along the line of `// return the VSTOffsetPlaceholder if we have a VST`
>
> ================
> Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2486
> @@ +2485,3 @@
> +    if (!isa<GlobalValue>(U2)) {
> +      findRefEdges(U2, VE, RefEdges, Visited);
> +      continue;
> ----------------
> tejohnson wrote:
> > joker.eph wrote:
> > > Guarantee on the recursion depth?
> > Beyond the Visited set check earlier? What would you like to see?
> I was not worried on non-termination or corretness, but depth that would
> explode the stack.
>
> It was late and I couldn't think clearly enough to find the answer myself.
> I just got a coffee so I should be able to answer myself now ;)
>
> -> For every instruction you will pull transitively all the operands until
> you reach a global (or something already seen). i.e. this is a DFS search
> on the SSA graph.
> If I'm correct, a worklist would probably be appropriate here.
>
>
> ================
> Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2560
> @@ +2559,3 @@
> +        for (const auto &OI : I->operands()) {
> +          if (CS && CS.isCallee(&OI)) {
> +            auto CalledFunction = CS.getCalledFunction();
> ----------------
> tejohnson wrote:
> > joker.eph wrote:
> > > `if(CS)` could be hoisted out of the loop.
> > Yes, missed this when I cleaned things up from when I was initially
> looking for non-call references inside this loop.
> Re-reading, I'm not even sure what this loop on the operand is doing at
> all!
> Wouldn't the code do exactly the same if you just remove the loop and turn
> the test into `if (CS)``
>
>
> ================
> Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2572
> @@ +2571,3 @@
> +              CallGraphEdges[CalleeId] += ScaledCount;
> +              RefEdges.erase(CalleeId);
> +            }
> ----------------
> 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.
>

I think the most important property is that the union of CG edges and Ref
edges should include all global values referenced in anyway by the
function. Unless we rely on Ref edges to be inclusive or exclusive, 1,2,3
do not matter much, but I think 2) is probably the best.

David


>
>
>
> http://reviews.llvm.org/D17212
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160308/3f19fd02/attachment.html>