[LLVMdev] Profiling in LLVM Patch Followup 1

[Andreas Neustifter]

Hi Daniel!

Daniel Dunbar wrote:
> I merged in my changes to your patch, which results in the attached
> patch. There may be some missed merge errors. The main problem I have
> with the rest of this patch is that it causes a regression in
> llvm-prof's behavior. I tried running edge profiling on the
> MultiSource/Applications/aha benchmark in the llvm test-suite, and
> llvm-prof no longer prints any function information:
> --
> ddunbar at giles:aha$ opt -f Output/aha.linked.rbc -insert-edge-profiling -o foo.bc
> ddunbar at giles:aha$ llc foo.bc -o - | gcc -x assembler - -x none
> ~/llvm/Debug/lib/profile_rt.dylib
> ddunbar at giles:aha$ ./a.out
> ...
> ddunbar at giles:aha$ ~/llvm/Release/bin/llvm-prof foo.bc llvmprof.out
> [...]
First of all, you have to use the original bytecode file
(Output/aha.linked.bc) for use with llvm-prof like this:

$ ~/llvm/Release/bin/llvm-prof Output/aha.linked.bc llvmprof.out

Also you have to check that llvmprof.out is deleted between tests
otherwise the figures are accumulated counts from several runs.

> [...]
> 
> The basic block counts also differ, sometimes very significantly, but
> I'm not sure if this is to be expected. Can you investigate both of
> these differences on some real example? I'd like to keep everything
> working at least as good as it was before (modulo some other features,
> like function counts or basic block traces, which we don't really
> support).

I have to compare the revisions 75622 and 75625 to check what's going on
there, but maybe its also due to undeleted llvmprof.out files?

> [...]
>> Index: include/llvm/Analysis/Passes.h
>> ===================================================================
>> --- include/llvm/Analysis/Passes.h	(revision 74697)
>> +++ include/llvm/Analysis/Passes.h	(working copy)
> ...
>> +  ModulePass *createProfileLoaderPass(ProfileInfoLoader *PIL);
> 
> I avoided the need for this by just loading the profile info twice, as
> a sort of intermediate step.
> 
>> +  std::vector<unsigned> ECs = PIL->getRawEdgeCounts();
> 
> This copies the vector, it should be a (const) refererence.
> 
>> +  // Instrument all of the edges...
>> +  unsigned i = 0;
>> +  for (Module::iterator F = M.begin(), E = M.end(); F != E; ++F)
>> +    for (Function::iterator BB = F->begin(), E = F->end(); BB != E; ++BB) {
>> +      // Okay, we have to add a counter of each outgoing edge.  If the
>> +      // outgoing edge is not critical don't split it, just insert the counter
>> +      // in the source or destination of the edge.
>> +      TerminatorInst *TI = BB->getTerminator();
>> +      for (unsigned s = 0, e = TI->getNumSuccessors(); s != e; ++s) {
>> +        EdgeCounts[std::make_pair(BB, TI->getSuccessor(s))]+= ECs[i++];
> 
> This can end up reading off the end of ECs if the module changes (or
> the profile information is of a different type).
> 
>> Index: tools/llvm-prof/llvm-prof.cpp
>> ===================================================================
>> --- tools/llvm-prof/llvm-prof.cpp	(revision 74697)
>> +++ tools/llvm-prof/llvm-prof.cpp	(working copy)
> 
>> +  class LoaderInterface : public ModulePass {
>> +    ProfileInfo *PI;
> 
> I renamed this class, and moved the main printing functionality to its
> run method. This avoids the member state.
> 
>>    class ProfileAnnotator : public AssemblyAnnotationWriter {
>> -    std::map<const Function  *, unsigned> &FuncFreqs;
>> -    std::map<const BasicBlock*, unsigned> &BlockFreqs;
>> -    std::map<ProfileInfoLoader::Edge, unsigned> &EdgeFreqs;
>> +    ProfileInfo *PI;
> 
> I would advocate making this a reference instead of a pointer; that
> makes it obvious that this class doesn't own the ProfileInfo, and that
> the client is responsible for its memory management.

No objections.

Thanks, Andi