[PATCH] Add function entry count metadata.

[Duncan P. N. Exon Smith]

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

(Review comments below.)

> 
> Thanks.
> 
> 
> http://reviews.llvm.org/D9628
> 
> Files:
>  include/llvm/IR/Function.h
>  include/llvm/IR/MDBuilder.h
>  lib/IR/MDBuilder.cpp
>  unittests/IR/MetadataTest.cpp
> 
> EMAIL PREFERENCES
>  http://reviews.llvm.org/settings/panel/emailpreferences/
> <D9628.25390.patch>

> Index: include/llvm/IR/Function.h
> ===================================================================
> --- include/llvm/IR/Function.h
> +++ include/llvm/IR/Function.h
> @@ -19,11 +19,16 @@
>  #define LLVM_IR_FUNCTION_H
>  
>  #include "llvm/ADT/iterator_range.h"
> +#include "llvm/ADT/Optional.h"

(You need this one, but...)

>  #include "llvm/IR/Argument.h"
>  #include "llvm/IR/Attributes.h"
>  #include "llvm/IR/BasicBlock.h"
>  #include "llvm/IR/CallingConv.h"
> +#include "llvm/IR/Constants.h"
>  #include "llvm/IR/GlobalObject.h"
> +#include "llvm/IR/LLVMContext.h"
> +#include "llvm/IR/MDBuilder.h"
> +#include "llvm/IR/Metadata.h"

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

>  #include "llvm/Support/Compiler.h"
>  
>  namespace llvm {
> @@ -194,6 +199,30 @@
>                                   AttributeSet::FunctionIndex, Kind, Value));
>    }
>  
> +  /// @brief Set the entry count for this function.

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!

> +  void setEntryCount(uint64_t Count) {
> +    MDBuilder MDB(getContext());
> +    setMetadata(LLVMContext::MD_prof, MDB.createFunctionEntryCount(Count));
> +  }
> +
> +  /// @brief Return true if the function has a known entry count.
> +  bool hasEntryCount() const {
> +    MDNode *MD = getMetadata(LLVMContext::MD_prof);
> +    if (MD && MD->getOperand(0))
> +      if (MDString* MDS = dyn_cast<MDString>(MD->getOperand(0)))
> +        return MDS->getString().equals("function_entry_count");
> +    return false;

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?

> +  }
> +
> +  /// @brief Get the entry count for this function.
> +  Optional<uint64_t> getEntryCount() const {
> +    if (!hasEntryCount())
> +      return None;

If you remove the string thing, you can drop this too.

> +    MDNode *MD = getMetadata(LLVMContext::MD_prof);
> +    ConstantInt *CI = mdconst::extract<ConstantInt>(MD->getOperand(1));
> +    return CI->getValue().getZExtValue();
> +  }
> +
>    /// @brief Return true if the function has the attribute.
>    bool hasFnAttribute(Attribute::AttrKind Kind) const {
>      return AttributeSets.hasAttribute(AttributeSet::FunctionIndex, Kind);
> Index: include/llvm/IR/MDBuilder.h
> ===================================================================
> --- include/llvm/IR/MDBuilder.h
> +++ include/llvm/IR/MDBuilder.h
> @@ -60,6 +60,9 @@
>    /// \brief Return metadata containing a number of branch weights.
>    MDNode *createBranchWeights(ArrayRef<uint32_t> Weights);
>  
> +  /// \brief Return metadata containing the entry count for a function.
> +  MDNode *createFunctionEntryCount(uint64_t Count);
> +
>    //===------------------------------------------------------------------===//
>    // Range metadata.
>    //===------------------------------------------------------------------===//
> Index: lib/IR/MDBuilder.cpp
> ===================================================================
> --- lib/IR/MDBuilder.cpp
> +++ lib/IR/MDBuilder.cpp
> @@ -53,6 +53,16 @@
>    return MDNode::get(Context, Vals);
>  }
>  
> +MDNode *MDBuilder::createFunctionEntryCount(uint64_t Count) {
> +  SmallVector<Metadata *, 2> Vals(2);
> +  Vals[0] = createString("function_entry_count");
> +
> +  Type *Int64Ty = Type::getInt64Ty(Context);
> +  Vals[1] = createConstant(ConstantInt::get(Int64Ty, Count));
> +
> +  return MDNode::get(Context, Vals);
> +}
> +
>  MDNode *MDBuilder::createRange(const APInt &Lo, const APInt &Hi) {
>    assert(Lo.getBitWidth() == Hi.getBitWidth() && "Mismatched bitwidths!");
>  
> Index: unittests/IR/MetadataTest.cpp
> ===================================================================
> --- unittests/IR/MetadataTest.cpp
> +++ unittests/IR/MetadataTest.cpp
> @@ -2272,4 +2272,12 @@
>    EXPECT_FALSE(verifyFunction(*F));
>  }
>  
> +TEST_F(FunctionAttachmentTest, EntryCount) {
> +  Function *F = getFunction("foo");
> +  EXPECT_FALSE(F->hasEntryCount());
> +  F->setEntryCount(12304);
> +  EXPECT_TRUE(F->hasEntryCount());
> +  EXPECT_EQ(12304u, *F->getEntryCount());
> +}
> +
>  }

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