On Apr 10, 2014, at 17:21, Duncan P. N. Exon Smith <[email protected]> wrote:
> 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. 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.
pgo-hash-5.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
