[LLVMdev] Profiling in LLVM Patch Followup 1

[Daniel Dunbar]

Hi Andreas,

Thanks for breaking things up.

I applied two pieces of this patch in separate no-functionality-change
commits, here:
  http://llvm.org/viewvc/llvm-project?view=rev&revision=75623
and here:
  http://llvm.org/viewvc/llvm-project?view=rev&revision=75625

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
WARNING: profile information is inconsistent with the current program!
===-------------------------------------------------------------------------===
LLVM profiling output for executions:
  1. ./a.out

===-------------------------------------------------------------------------===
Function execution frequencies:

 ##   Frequency

  NOTE: 32 functions were never executed!

--
whereas before I would get something like:
--
===-------------------------------------------------------------------------===
Function execution frequencies:

 ##   Frequency
  1. 87161541/406099952 selle
  2. 86016884/406099952 print_pgm
  3. 80066663/406099952 check
  4. 44786540/406099952 fix_operands
  5. 17224823/406099952 divide
  6. 13735854/406099952 increment
...
--

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).

Some minor comments on the (original) patch below...

Thanks,
 - Daniel

> 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.

On Thu, Jul 2, 2009 at 8:23 AM, Andreas
Neustifter<e0325716 at student.tuwien.ac.at> wrote:
> Hi,
>
> this is the first in a series of patches to cleanup and improve the LLVM
> Profiling Infrastructure.
>
> First and foremost this patch removes duplicate functionality from
> ProfileInfoLoader and ProfileInfo:
> The ProfileInfoLoader performed not only the loading of the profile
> information but also some synthesis of block and function execution counts
> from edge profiling information. Since the ProfileInfo performs this
> synthesis anyways, the ProfileInfoLoader was demoted to be really only the
> interface to the profile information file.
>
> Since the llvm-prof was the only client of the synthesis portion of the
> ProfileInfoLoader it had to be changed to fetch this kind of information
> from the ProfileInfo with the help of the ProfileInfoLoaderPass.
>
> The next step are to
> *) add proper handling of an edge (0,entry) for each function to the
> Profiling Infrastructure
> *) provide a dedicated return value if the profiling information for a
> certain element is not available
> *) change the profiling values itself to double as preparation for future
> passes that require floating point support from ProfileInfo
>
> Greetings, Andi
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm-r74697.profileinfo.cleanup.regenerated.patch
Type: application/octet-stream
Size: 19137 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090714/89b5bbc2/attachment.obj>