[PATCH] InstrProf: Calculate a better function hash

[Duncan P. N. Exon Smith]

On 2014-Apr-11, at 14:23, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:

> 
> On Apr 10, 2014, at 17:21, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
>> On Apr 10, 2014, at 4:13, Chandler Carruth <chandlerc at google.com> wrote:
>> 
>>> And comments on the proper change...
>> 
>> Thanks for the review!  Updated patch attached.
>> 
>>> +void MapRegionCounters::assignCounter(const Decl *D) {
>>> +  CounterMap[D->getBody()] = NextCounter++;
>>> +  Hash.combine(hashDecl(*D));
>>> +}
>>> 
>>> Why hash the declaration at all? There shouldn't be any cases where a declaration impacts control flow, and you have complexity enum and in the hasher to handle this.
>> 
>> Good point.
>> 
>>> +static PGOHash::HashType hashStmt(const Stmt &S) {
>>> 
>>> While I see the appeal of this abstraction, I don't think it is ultimately buying you much of anything. We already do a full switch over the StmtClass in the visitor, we shouldn't re-do it here (for simplicity, I'm moderately confident the optimizer will "fix" this). I would probably leave the counter management as it was, and just add a hash combine step for the statements. Or you could pass the enum through and share the counter growth and hash combination much like you have in the current patch. The complexity I would avoid is yet another switch over all of the CFG-impacting statement kinds.
>>> 
>>> Does that make sense to you?
>> 
>> I'm mainly concerned about minimizing boilerplate.  Bob's working on a
>> patch to switch to RecursiveASTVisitor so we can gut the traversal
>> logic, and I don't want to move in the opposite direction.
>> 
>> Your idea to pass the enum through accomplishes the same goal (and is
>> obviously cleaner), so I went with that.
> 
> I've rebased after Bob's commit in r206039.  New patch attached.
> 
> I re-added a getHashStmt() function since it made for the cleanest
> diff.  I think this is the best way here after all.

Rebased for Justin's recent changes.  New patch attached.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pgo-hash-6.patch
Type: application/octet-stream
Size: 27045 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140415/83c0bde4/attachment.obj>