[PATCH] Add function entry count metadata.

[Diego Novillo]

On Sat, May 9, 2015 at 6:35 AM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2015 May 8, at 19:39, Diego Novillo <dnovillo at google.com> wrote:
>>
>> Return Optional<uint64_t> from getEntryCount to indicate whether a count is available.
>>
>> I'm getting several odd failures in the testsuite. Even though the new code
>> is only called from the unittest, I'm getting several related failures.
>> For example,
>>
>> bin/opt -S test/Verifier/statepoint.ll -verify
>> ----------------------------------------------
>>
>> Exit Code: 2
>>
>> Command Output (stderr):
>> ------------------------
>>
>> gc.statepoint mismatch in number of vararg call args
>>
>>  %safepoint_token = call i32 (void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(void ()* undef, i32 0, i32 0, i32 0, i32 5, i32 0, i32 0, i32 0, i32 10, i32 0, i8 addrspace(1)* %arg, i64 addrspace(1)* %cast, i8 addrspace(1)* %arg, i8 addrspace(1)* %arg)
>>
>> gc.statepoint mismatch in number of vararg call args
>>
>>  %safepoint_token = call i32 (void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(void ()* undef, i32 0, i32 0, i32 0, i32 5, i32 0, i32 0, i32 0, i32 10, i32 0, i8 addrspace(1)* %arg, i64 addrspace(1)* %cast, i8 addrspace(1)* %arg, i8 addrspace(1)* %arg)
>>
>> gc.statepoint mismatch in number of vararg call args
>>
>>  %0 = invoke i32 (void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(void ()* undef, i32 0, i32 0, i32 0, i32 5, i32 0, i32 -1, i32 0, i32 0, i32 0, i8 addrspace(1)* %obj, i8 addrspace(1)* %obj1)
>>          to label %normal_dest unwind label %exceptional_return
>>
>> It seems related to metadata, but I'm not sure how my patch could've triggered
>> this. Am I missing anything obvious?
>
> I don't see anything obvious, and this makes no sense to me :(.
> I hope there's something wrong with your tree.

Yeah. My tree was busted. All good now.

> Please avoid adding any of these includes.  `Function.h` is
> intentionally kept as small as possible.  (That just means defining the
> functions in the source file instead of inline.)

Done.

> Recently on llvmdev we decided to turn on a doxygen feature that renders
> `@brief` and `\brief` unnecessary.  Instead, doxygen will auto-brief the
> first paragraph (like a git commit message).  Please leave this out!

Oh, OK. I cargo-culted this from the other comments in the same file.
Sounds like a cleanup is needed.

> I don't think this string serves a useful purpose.  We can define the
> `!prof` function attachment to mean "function entry count".  I'd rather
> see the more barebones:
>
>     define void @foo() !prof !0 {
>     ...
>     }
>
>     !0 = !{i32 92834}
>
> In which case, `hasFunctionCount()` can just be:
>
>     bool hasEntryCount() const { return getEntryCount(); }
>
> Unless there's some other profile information anyone can envision adding
> as a function attachment *instead* of a function entry count?

There may be others in the future and putting a name to it makes it
clear to the casual reader, and other tools that look at bytecode
files (say grep when you're debugging, etc).

Are you really set against having a name? I would much rather see the
slightly more verbose metadata. It's already hard to read metadata
nodes as it is.

> Missing: Verifier checks for this attachment (although if this comes in
> a follow-up patch, that's fine).  You should check that !prof function
> attachments are structured exactly correctly (MDTuple with one operand,
> which is `ConstantAsMetadata`, which holds a `ConstantInt`).  These
> tests usually go in test/Verifier/.

Sure. I'll add these too in this patch. Update coming up.


Thanks.  Diego.