<div dir="ltr">And comments on the proper change...<div><br></div><div><div>+void MapRegionCounters::assignCounter(const Decl *D) {</div><div>+ CounterMap[D->getBody()] = NextCounter++;</div><div>+ Hash.combine(hashDecl(*D));</div>
<div>+}</div></div><div><br></div><div>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.</div><div><br>
</div><div><div>+static PGOHash::HashType hashStmt(const Stmt &S) {</div></div><div><br></div><div>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.</div>
<div><br></div><div>Does that make sense to you?</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Apr 10, 2014 at 11:56 AM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Another set of diffs that really should be pulled out into a separate commit:<div><br></div><div><div> /// Assign a counter to track entry to the function body.</div>
<div>- void VisitFunctionDecl(const FunctionDecl *S) {</div>
<div>- CounterMap[S->getBody()] = NextCounter++;</div><div>- Visit(S->getBody());</div><div>+ void VisitFunctionDecl(const FunctionDecl *D) {</div><div>+ assignCounter(D);</div><div>+ Visit(D->getBody());</div>
<div> }</div></div><div><br></div><div>All of the simple parameter renames are fine, just commit those. No need to clutter up the functional change.</div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra">
<br><br><div class="gmail_quote">
On Thu, Apr 10, 2014 at 11:47 AM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">Still looking at the patch but:<div><br></div><div><div><div> Using hash values with the most significant bit set exposed a bug where</div></div><div> unsigned values were read using strtol and strtoll. Changed to strtoul</div>
<div> and strtoull to keep the tests passing.</div></div><div><br></div><div>This bit is clearly correct, and I'm happy for it to get exercised once the final bits land. I would split this out into its own patch and go ahead and commit it. The hash change is better as a separate patch IMO.</div>
</div><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Apr 10, 2014 at 11:45 AM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra">Sorry for delays, took vacation around EuroLLVM... I'll try to catch up on some of these, but will probably only hit full steam again next week.</div>
<div class="gmail_extra"><div><br>
<div class="gmail_quote">On Wed, Mar 26, 2014 at 11:27 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 style="overflow:hidden">The hash here (Working) is just like any other number in IRGen. Its byte-<br>
swapping requirements are no different from, e.g.,<br>
CodeGenPGO::NumRegionCounters.<br>
<br>
Here’s the flow (with the current file formats):<br>
<br>
- IRGen emits an i64 as part of the __llvm_profile_data_foo aggregate.<br>
<br>
- CodeGen emits it in the right section, byte swapping if host!=target.<br>
<br>
- The profile runtime dumps it in the raw binary format. This code is in<br>
compiler-rt:lib/profile/InstrProfilingFile.c and<br>
compiler-rt:lib/profile/InstrProfilingBuffer.c.<br>
<br>
- llvm-profdata reads it in, byte swapping if host!=target. This code is<br>
in llvm:lib/ProfileData/InstrProfReader.cpp.</div></blockquote></div><br></div>Ahh, ok. The part I was missing here was that the raw format is target-endian rather than endian neutral. That makes a lot of sense (for the same reason I originally pushed for having the raw format be different from the merged final format), but it just didn't click initially that this is why everything works.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">I'll take another look at the patch.</div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>