Re: [GOOGLE] Refactor AutoFDO

[Xinliang David Li <davidxl at google dot com>]

I have some preliminary comments. Mostly just related to code style
and missing documentation.

David

>
> #define DEFAULT_AUTO_PROFILE_FILE "fbdata.afdo"
>
> struct SourceLocation

Is using Upper case in struct names allowed?

> {
>   tree func_decl;
>   unsigned lineno;
> };
>
> typedef std::vector<const char *> StringVector;
> typedef std::vector<SourceLocation> InlineStack;
> typedef std::map<unsigned, gcov_type> TargetMap;
>

Add short description of each new types.

> struct ProfileInfo
> {
>   gcov_type count;
>   TargetMap target_map;
> };
>
> struct StringCompare
> {
>   bool operator() (const char* a, const char* b) const

'*' should bind to name.

>     { return strcmp (a, b) < 0; }
> };
>

> class StringMap {
>  public:
>   static StringMap *Create();
>   int GetIndex (const char *name) const;
>   int GetIndexByDecl (tree decl) const;
>   const char *GetName (int index) const;
>
>  private:
>   StringMap () {}
>   bool Read ();
>
>   typedef std::map<const char *, unsigned, StringCompare> StringIndexMap;
>   StringVector vector_;
>   StringIndexMap map_;
> };

Add some documentation on the new type, indicating what is 'index'.

>
> class Symbol {

The name 'Symbol' is too generic -- can cause conflicts in the future
unless namespace is used. ALso missing documentation.

>  public:
>   typedef std::vector<Symbol *> SymbolStack;

Fix indentation problems.


>
>   /* Read the profile and create a symbol with head count as  HEAD_COUNT.
>      Recursively read callsites to create nested symbols too. STACK is used
>      to track the recursive creation process.  */
>   static const Symbol *ReadSymbol (SymbolStack *stack, gcov_type head_count);
>
>   /* Recursively deallocate all callsites (nested symbols).  */
>   ~Symbol ();
>
>   /* Accessors.  */
>   unsigned name () const { return name_; }
>   gcov_type total_count () const { return total_count_; }
>   gcov_type head_count () const { return head_count_; }
>
>   /* Recursively traverse STACK starting from LEVEL to find the corresponding
>      symbol.  */
>   const Symbol *GetSymbol (const InlineStack &stack, unsigned level) const;
>
>   /* Return the profile info for LOC.  */
>   bool GetProfileInfo (location_t loc, ProfileInfo *info) const;
>
>  private:
>   Symbol (unsigned name, gcov_type head_count)
>       : name_(name), total_count_(0), head_count_(head_count) {}
>   const Symbol *GetSymbolByDecl (unsigned lineno, tree decl) const;
>
>   typedef std::map<gcov_type, const Symbol *> CallsiteMap;

Need documentation for this map.

>   typedef std::map<unsigned, ProfileInfo> PositionCountMap;

Need documentation.

>
>   /* Symbol name index in the string map.  */
>   unsigned name_;
>   /* The total sampled count.  */
>   gcov_type total_count_;
>   /* The total sampled count in the head bb.  */
>   gcov_type head_count_;
>   /* Map from callsite location to callee symbol.  */
>   CallsiteMap callsites;
>   /* Map from source location to count and instruction number.  */
>   PositionCountMap pos_counts;
> };
>
> class SymbolMap {

Need documentation.

>  public:
>   static SymbolMap *Create ()
>     {
>       SymbolMap *map = new SymbolMap ();
>       if (map->Read ())
> return map;
>       delete map;
>       return NULL;
>     }
>   ~SymbolMap ();
>   const Symbol *GetSymbolByDecl (tree decl) const;
>   bool GetProfileInfo (gimple stmt, ProfileInfo *info) const;
>   gcov_type GetCallsiteTotalCount (struct cgraph_edge *edge) const;

 Missing documentation for the interfaces
>
>  private:

>   typedef std::map<unsigned, const Symbol *> NameSymbolMap;

map from what to symbol?

>
>   SymbolMap () {}
>   bool Read ();
>   const Symbol *GetSymbolByInlineStack (const InlineStack &stack) const;

 Missing documentation for the interfaces


>
>   NameSymbolMap map_;
> };
>
> class ModuleMap {

Need documentation.

On Tue, Jul 30, 2013 at 11:03 AM, Dehao Chen <dehao@google.com> wrote:
> I just rebased the CL to head and updated the patch.
>
> Thanks,
> Dehao
>
> On Tue, Jul 30, 2013 at 10:09 AM, Xinliang David Li <davidxl@google.com> wrote:
>> I can not apply the patch cleanly in my v17 gcc client -- there is
>> some problem in auto-profile.c.
>>
>> David
>>
>> On Mon, Jul 29, 2013 at 7:52 PM, Dehao Chen <dehao@google.com> wrote:
>>> This patch refactors AutoFDO to use:
>>>
>>> 1. C++ to rewrite the whole thing.
>>> 2. Use tree instead of hashtable to represent the profile.
>>> 3. Make AutoFDO standalone: keep changes to other modules minimum.
>>>
>>> Bootstrapped and passed regression test and benchmark test.
>>>
>>> OK for google-4_8 branch?
>>>
>>> Thanks,
>>> Dehao
>>>
>>> http://codereview.appspot.com/12079043