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

Teresa Johnson via llvm-commits llvm-commits at lists.llvm.org
Tue Mar 8 13:21:30 PST 2016


tejohnson added inline comments.

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:852
@@ -849,2 +851,3 @@
+    VSTOffsetPlaceholder = WriteValueSymbolTableForwardDecl(Stream);
   return VSTOffsetPlaceholder;
 }
----------------
joker.eph wrote:
> 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`
Will do.

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2486
@@ +2485,3 @@
+    if (!isa<GlobalValue>(U2)) {
+      findRefEdges(U2, VE, RefEdges, Visited);
+      continue;
----------------
joker.eph wrote:
> 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.
>  
I wouldn't have thought that an instruction would be large enough to cause stack explosion. But I have no issue with changing this to a worklist iteration instead.

================
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();
----------------
joker.eph wrote:
> 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)``
> 
Right, this loop is bogus!

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2572
@@ +2571,3 @@
+              CallGraphEdges[CalleeId] += ScaledCount;
+              RefEdges.erase(CalleeId);
+            }
----------------
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.


http://reviews.llvm.org/D17212





More information about the llvm-commits mailing list