[LLVMdev] Profiling in LLVM Patch Followup 1

[Andreas Neustifter]

Hi!

Daniel Dunbar wrote:
> Applied as r78477.
> 
> I do have a few comments on the patch, below, but I'll wait to see
> where things are going before acting on them. :)
> 
> --
> 
>> Index: include/llvm/Analysis/ProfileInfo.h
>> ===================================================================
> 
>> +    // MissingValue - The value that is returned for execution counts in case
>> +    // no value is available.
>> +    static const int MissingValue = -1;
> 
> This should be a double?

Yes it should be. Since C++ (for some reason I can not fathom) does not allow to use a static const float this was my workaround. Is there a better solution?


>> +    // getFunction() - Returns the Function for an Edge, checking for validity.
>> +    static const Function* getFunction(Edge e) {
>> +      assert(e.second && "Invalid ProfileInfo::Edge");
>> +      return e.second->getParent();
>> +    }
>> +
>> +    // getEdge() - Creates an Edge from two BasicBlocks.
>> +    static Edge getEdge(const BasicBlock* Src, const BasicBlock* Dest) {
>> +      return std::make_pair(Src,Dest);
>> +    }
> 
> I think these should be private; eventually they are just an
> implementation detail of a particular ProfileInfo provider.

My plan was to have Edges as integral part of the ProfileInfo since they only store information for a given CFG. An provider of ProfileInformation can then decide at which level information is provided. If possible the ProfileInfo calculates other information from that. (E.g. calculate block counts from edge counts.) This has the advantage that not every ProfileInfo provider has to reimplement the logic for deriving additional information.


>> -  // We don't want to create an std::set unless we are dealing with a block that
>> -  // has a LARGE number of in-edges.  Handle the common case of having only a
>> -  // few in-edges with special code.
> 
> Thank you for killing this premature optimization. :)

Well, when not caching the results it was good I guess. But with a results cache thats not necessary any more.


>> +  // the sum of the edge frequencies from the incoming edges.
> 
> the -> The

Even if its part of an sentence? (See lib/Analysis/ProfileInfo.cpp, line 49.)


> This code also makes me thing MissingValue should be renamed,
> MissingValue is something like "NoData", whereas MissingValue could
> easily be interpreted as NeverExecuted. I think there should probably
> be a comment in the doc for this method that the count goes to
> MissingValue if any edge is missing data.

I will think about how to solve this best.

Thank you so much!

Andi