[PATCH] InstrProf: Calculate a better function hash

[Chandler Carruth]

And comments on the proper change...

+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.

+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?



On Thu, Apr 10, 2014 at 11:56 AM, Chandler Carruth <chandlerc at google.com>wrote:

> Another set of diffs that really should be pulled out into a separate
> commit:
>
>      /// Assign a counter to track entry to the function body.
> -    void VisitFunctionDecl(const FunctionDecl *S) {
> -      CounterMap[S->getBody()] = NextCounter++;
> -      Visit(S->getBody());
> +    void VisitFunctionDecl(const FunctionDecl *D) {
> +      assignCounter(D);
> +      Visit(D->getBody());
>      }
>
> All of the simple parameter renames are fine, just commit those. No need
> to clutter up the functional change.
>
>
> On Thu, Apr 10, 2014 at 11:47 AM, Chandler Carruth <chandlerc at google.com>wrote:
>
>> Still looking at the patch but:
>>
>>     Using hash values with the most significant bit set exposed a bug
>> where
>>     unsigned values were read using strtol and strtoll.  Changed to
>> strtoul
>>     and strtoull to keep the tests passing.
>>
>> 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.
>>
>>
>> On Thu, Apr 10, 2014 at 11:45 AM, Chandler Carruth <chandlerc at google.com>wrote:
>>
>>> 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.
>>>
>>> On Wed, Mar 26, 2014 at 11:27 PM, Duncan P. N. Exon Smith <
>>> dexonsmith at apple.com> wrote:
>>>
>>>> The hash here (Working) is just like any other number in IRGen.  Its
>>>> byte-
>>>> swapping requirements are no different from, e.g.,
>>>> CodeGenPGO::NumRegionCounters.
>>>>
>>>> Here’s the flow (with the current file formats):
>>>>
>>>>   - IRGen emits an i64 as part of the __llvm_profile_data_foo aggregate.
>>>>
>>>>   - CodeGen emits it in the right section, byte swapping if
>>>> host!=target.
>>>>
>>>>   - The profile runtime dumps it in the raw binary format.  This code
>>>> is in
>>>>     compiler-rt:lib/profile/InstrProfilingFile.c and
>>>>     compiler-rt:lib/profile/InstrProfilingBuffer.c.
>>>>
>>>>   - llvm-profdata reads it in, byte swapping if host!=target.  This
>>>> code is
>>>>     in llvm:lib/ProfileData/InstrProfReader.cpp.
>>>>
>>>
>>> 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.
>>>
>>> I'll take another look at the patch.
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140410/bd10b299/attachment.html>