On 2014-Apr-11, at 14:23, Duncan P. N. Exon Smith <[email protected]> wrote:
> > 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. Rebased for Justin's recent changes. New patch attached.
pgo-hash-6.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
