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