[llvm-commits] [PATCH 2/4] Profile metadata: Profile loader

[Alastair Murray]

Hi Manman,

Attached is a reworked version of this patch to address #1 and #2 of 
your comments.

I hope this is what you meant by #1, most of LLVM does not seem to use 
the more extensive parts of doxygen (@param, @return etc), so I haven't 
either.  If you would like this then please let me know.   What I have 
done is larger change some '//' comments to '///', made occasional use 
of '\brief' and added a few extra comments.

I'm currently working on #4, which I will send as a separate patch.  #3 
will also be sent as a separate patch, though not until after fixing 
switch weights patch (separate thread).

Regards,
Alastair.


On 22/08/12 21:05, Manman Ren wrote:
>
> Hi Alastair,
>
> Thanks for the patch. It looks good.
>
> I have a few comments:
> 1> I prefer doxygen comment block for the classes and functions :)
> 2> Should we get rid of "; preds " in your testing cases?
> 3> Should we also update ProfileVerifierPass to use ProfileDataT instead of ProfileInfoT?
> 4> Can we update the definition of an edge here?
> +class ProfileDataT {
> +  public:
> +  // The profiling information defines an Edge by its source and sink basic
> +  // blocks.
> +  typedef std::pair<const BType*, const BType*> Edge;
> +
> +  private:
> +  typedef std::map<Edge, unsigned> EdgeWeights;
>
> In the following example:
>    for (int i = 0; i < STRLEN; ++i)
>      for (int j = 0; j < STRLEN; ++j)
>        switch(strs[i][j]) {
>        default:
>          sum += strs[i][j];
>          break;
>        case 0:
>        case 1:
>        case 2:
>          // Trying to make the counter different for these 3 cases.
>          sum += strs[i][j] * 2;
>          break;
>        case 5:
>          sum += strs[i][j] * 4;
>          break;
>        }
>
> We will have 3 counters from the switch block to the target of case 0, case 1 and case 2, but I believe the old ProfileInfo and your implementation
> will associate the same weight for the three cases. Since all 3 edges are between the same pair of blocks, we can't tell the difference with edge
> defined as a pair of blocks.
>
> That said, I am okay with submitting additional patches to fix 3) and 4) later on.
>
> Thanks,
> Manman
>
> On Aug 18, 2012, at 6:37 AM, Alastair Murray wrote:
>
>> Hello all,
>>
>> Please review the attached patch.
>>
>> This is the second of four patches to support setting branch weight metadata by profiling.  It depends on the first patch.
>>
>> There are also three binary profile data files attached that are used for testing, these should go in test/Analysis/Profiling.
>>
>> This patch implements the profile loader which can load profile data generated by programs compiled with "-insert-edge-profiling".  It also provides tests and moves three function definitions out of ProfileInfo into the new profiling code.  The pass is invoked by "-profile-metadata-loader", but I believe that once ProfileInfo etc. is removed it should take over the "-profile-loader" switch that that currently uses.
>>
>> The attached patch has been cleaned up a little from the version sent to llvm-dev last week, some class renaming, commenting etc.  It is largely the same in logical terms though.
>>
>> It has been tested via the attached tests and by '-profile-metadata-loader -std-compile-opts' over test-suite (using the Makefile to come in patch #4).
>>
>> Regards,
>> Alastair.
>> <weights_2_profiler_loader.patch><load-branch-weights-ifs.ll.prof_data><load-branch-weights-loops.ll.prof_data><load-branch-weights-switches.ll.prof_data>_______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
> .
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: weights_2_profiler_loader.patch
Type: text/x-patch
Size: 36720 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120828/dd85626d/attachment.bin>