[PATCH] Add function entry count metadata.

[Duncan P. N. Exon Smith]

> On 2015-May-12, at 16:02, Diego Novillo <dnovillo at google.com> wrote:
> 
> On Tue, May 12, 2015 at 6:49 PM, Duncan P. N. Exon Smith
> <dexonsmith at apple.com> wrote:
>> 
>>> On 2015-May-12, at 14:53, Philip Reames <listmail at philipreames.com> wrote:
>>> 
>>> Reading through the code, I was a left a bit confused as to the actual structure.  Could you update with the LangRef update so that it's easier to review?  Also, tests for correct usage would be helpful.
>>> 
>>> 
>>> ================
>>> Comment at: lib/IR/Function.cpp:995
>>> @@ +994,3 @@
>>> +  MDBuilder MDB(getContext());
>>> +  setMetadata(LLVMContext::MD_prof, MDB.createFunctionEntryCount(Count));
>>> +}
>>> ----------------
>>> I'm mildly bothered by using the same name for two different pieces of metadata.  Is there a reason not to use a different name here?
>>> 
>> 
>> I'm fine with it.  Having the string (also for "branch_weights") bothers
>> me intuitively since it blocks MDNode uniquing, but I guess it's better
>> that you're consistent with what's there (and it's not like this is a
>> major memory problem or anything).  Besides, if/when I can come up with
>> a well-reasoned argument to remove it, it'll be trivial to upgrade out.
> 
> Sounds good. Thanks.
> 
>> LGTM once you add the LangRef stuff Philip noticed.
> 
> OK, thanks.

Actually I just noticed a couple of minor things in the Verifier
(I don't need to see it again, but please fix before commit):

Index: lib/IR/Verifier.cpp
===================================================================
--- lib/IR/Verifier.cpp
+++ lib/IR/Verifier.cpp
> @@ -1463,6 +1465,33 @@
>    }
>  }
>  
> +void Verifier::VerifyFunctionMetadata(
> +    const SmallVector<std::pair<unsigned, MDNode *>, 4> MDs) {
> +  if (MDs.empty())
> +    return;
> +
> +  for (unsigned i = 0; i < MDs.size(); i++) {
> +    if (MDs[i].first == LLVMContext::MD_prof) {
> +      MDNode *MD = MDs[i].second;
> +      Assert(MD->getNumOperands() >= 2,
> +             "!prof annotations should have at least 2 operands", MD);
> +      Assert(isa<MDString>(MD->getOperand(0)),
> +             "expected string with name of the !prof annotation", MD);

Check for null, too (`isa<>` asserts on null).

> +      MDString *MDS = dyn_cast<MDString>(MD->getOperand(0));
> +      StringRef ProfName = MDS->getString();
> +      if (ProfName.equals("function_entry_count")) {

This all seems unnecessarily complicated.  Might as well assert
that it's "function_entry_count", and if/when we have something
else we can add other logic.

Similarly, just change the earlier check to `== 2`.

> +        Assert(MD->getNumOperands() == 2,
> +               "expected exactly one argument for function_entry_count", MD);
> +        Assert(isa<ConstantAsMetadata>(MD->getOperand(1)),
> +               "expected integer argument to function_entry_count", MD);

Check for null, too (`isa<>` asserts on null).

> +      } else {
> +        Assert(false, StringRef("unexpected profile annotation ") + ProfName,
> +               MD);
> +      }
> +    }
> +  }
> +}
> +
>  void Verifier::VerifyConstantExprBitcastType(const ConstantExpr *CE) {
>    if (CE->getOpcode() != Instruction::BitCast)
>      return;
> 



> Philip, I've added some content to the docs. Please let me
> know it's enough or you want more details.
> 
> I think we may want to reorg the docs a bit, since
> function_entry_count and branch_weights are related but they should be
> in a "Profile" section, not in a "branch weights" section. I'll
> address this in a future patch.
> 
> 
> Diego.