[PATCH] InstrProf: Calculate a better function hash

[Duncan P. N. Exon Smith]

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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pgo-hash-4.patch
Type: application/octet-stream
Size: 29046 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140410/0305d90d/attachment.obj>