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

[Alastair Murray]

Hi Manman,

Thank you very much for the review.

#1 and #2 are obviously easy to fix, I'll get that done shortly.

#3 is quite doable, though I'm not sure how much overhaul would be 
required.  The branch weight metadata does not store as much information 
as ProfileInfo, so the verification will need to be modified appropriately.

#4 is interesting, I think my new implementation may actually get the 
weights wrong for this as the BB->BB count will be taken as the weight 
for each case.  As long as the critical edge splitting in 
insert-edge-profiling correctly allows each case to be counted this 
should not be too hard to fix.  I'll look into that.

My preference would be to solve #3 in a separate patch, but I'm not sure 
if that would be appropriate for #4 (as it can get weights wrong).

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