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

[Evan Cheng]

Any reason you are using std::vector and std::map (blah) rather than LLVM's ADT implementations?

evan

On Aug 28, 2012, at 2:25 PM, Alastair Murray <alastairmurray42 at gmail.com> wrote:

> Hi Manman,
> 
> No problem.  Updated patch is attached.
> 
> The test cases now generate the profiling data.  The tests are also still parallel safe (no llvmprof.out file collisions, the tests now have argc/argv so that they can take the -llvmprof-output argument).
> 
> Regards,
> Alastair.
> 
> On 28/08/12 21:12, manman ren wrote:
>> 
>> Hi Alastair,
>> 
>> I do not think it is a good idea to submit binary files such as "*prof_data". Can we generate those in the testing case and verify the loader with the generated data files?
>> 
>> Thanks,
>> Manman
>> 
>> On Aug 28, 2012, at 11:32 AM, Alastair Murray <alastairmurray42 at gmail.com> wrote:
>> 
>>> 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
>>>> 
>>>> .
>>>> 
>>> 
>>> <weights_2_profiler_loader.patch>
>> 
>> 
> 
> <weights_2_profiler_loader.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits