On Apr 10, 2014, at 4:13, Chandler Carruth <[email protected]> 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.
pgo-hash-4.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
