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 <de...@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 <davi...@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 <de...@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
