<div dir="ltr"><div>I don't really understand the use of RAV here, or the logic of having a secondary switch, but at this point, there are too many patches flying and I feel like there is some more extensive discussion that took place somewhere other than this thread. =/ No fun when you get bounced back and forth in code review.</div>
<div><br></div><div>I'm happy to go with the implementation strategy as you have it. I may even write up a patch to show how I'm imagining you might simplify this. Anyways...</div><div><br></div><div>Some more microscopic nits. Feel free to commit whenever. I like where this design has ended up, and thanks for seeing it through. =]</div>
<div><br></div><div>+/// \brief Stable hasher for PGO region counters.</div><div>+///</div><div>+/// PGOHash produces a stable hash of a given function's control flow.</div><div>+///</div><div>+/// Changing the output of this hash will invalidate all previously generated</div>
<div>+/// profiles -- i.e., don't do it.</div><div>+///</div><div>+/// \note When this hash does eventually change (years?), we still need to</div><div>+/// support old hashes. We'll need to pull in the version number from the</div>
<div>+/// profile data format and use the matching hash function.</div><div>+class PGOHash {</div><div>+ constexpr static int NumBitsPerType = 6;</div><div>+ constexpr static unsigned NumTypesPerWord =</div><div>+ sizeof(uint64_t) * 8 / NumBitsPerType;</div>
<div>+ constexpr static unsigned TooBig = 1u << NumBitsPerType;</div><div><br></div><div>I would sink these to live down with the static_assert, and sink all of it into the existing private section with member variables.</div>
<div><br></div><div>+</div><div>+public:</div><div>+ /// \brief Hash values for AST nodes.</div><div>+ ///</div><div>+ /// Distinct values for AST nodes that have region counters attached.</div><div>+ ///</div><div>+ /// These values must be stable. All new members must be added at the end,</div>
<div>+ /// and no members should be removed. Changing the enumeration value for an</div><div>+ /// AST node will affect the hash of every function that contains that node.</div><div>+ enum HashType : unsigned char {</div>
<div>+ None = 0,</div><div>+ LabelStmt = 1,</div><div>+ WhileStmt,</div><div>+ DoStmt,</div><div>+ ForStmt,</div><div>+ CXXForRangeStmt,</div><div>+ ObjCForCollectionStmt,</div><div>+ SwitchStmt,</div>
<div>+ CaseStmt,</div><div>+ DefaultStmt,</div><div>+ IfStmt,</div><div>+ CXXTryStmt,</div><div>+ CXXCatchStmt,</div><div>+ ConditionalOperator,</div><div>+ BinaryOperatorLAnd,</div><div>+ BinaryOperatorLOr,</div>
<div>+ BinaryConditionalOperator,</div><div>+</div><div>+ // Keep this last. It's for the static assert that follows.</div><div>+ LastHashType</div><div>+ };</div><div>+ static_assert(LastHashType <= TooBig, "Too many types in HashType");</div>
<div>+</div><div>+private:</div><div>+ uint64_t Working;</div><div>+ unsigned Count;</div><div>+ llvm::MD5 MD5;</div><div>+</div><div>+public:</div><div>+ // TODO: When this format changes, take in a version number here, and use the</div>
<div>+ // old hash calculation for file formats that used the old hash.</div><div>+ PGOHash() : Working(0), Count(0) {}</div><div>+ void combine(HashType Type);</div><div>+ uint64_t finalize();</div><div><br></div><div>
I would just put these up with the public enum. Excepting private types or enums which are necessary to define prior to the public interface (because it is defined in terms of them), I generally prefer the public interface at the top followed by the private interface. Mild preference though.</div>
<div><br></div><div>+};</div><div><br></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Apr 15, 2014 at 2:46 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
On 2014-Apr-11, at 14:23, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
<br>
><br>
> On Apr 10, 2014, at 17:21, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
><br>
>> On Apr 10, 2014, at 4:13, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br>
>><br>
>>> And comments on the proper change...<br>
>><br>
>> Thanks for the review! Updated patch attached.<br>
>><br>
>>> +void MapRegionCounters::assignCounter(const Decl *D) {<br>
>>> + CounterMap[D->getBody()] = NextCounter++;<br>
>>> + Hash.combine(hashDecl(*D));<br>
>>> +}<br>
>>><br>
>>> 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.<br>
>><br>
>> Good point.<br>
>><br>
>>> +static PGOHash::HashType hashStmt(const Stmt &S) {<br>
>>><br>
>>> 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.<br>
>>><br>
>>> Does that make sense to you?<br>
>><br>
>> I'm mainly concerned about minimizing boilerplate. Bob's working on a<br>
>> patch to switch to RecursiveASTVisitor so we can gut the traversal<br>
>> logic, and I don't want to move in the opposite direction.<br>
>><br>
>> Your idea to pass the enum through accomplishes the same goal (and is<br>
>> obviously cleaner), so I went with that.<br>
><br>
> I've rebased after Bob's commit in r206039. New patch attached.<br>
><br>
> I re-added a getHashStmt() function since it made for the cleanest<br>
> diff. I think this is the best way here after all.<br>
<br>
</div></div>Rebased for Justin's recent changes. New patch attached.<br>
<br>
</blockquote></div><br></div>