PGO: Statically generate data structures

[Justin Bogner]

"Duncan P. N. Exon Smith" <dexonsmith at apple.com> writes:
> I’ve attached updated patches that address your comments.

Thanks. This looks good to commit with one or two more little cleanups.

>>> +static StringRef getDataSection(const CodeGenModule &CGM) {
>>> +  if (CGM.getTarget().getTriple().getObjectFormat() == llvm::Triple::MachO)
>>> +    return "__DATA,__llvm_pgo_data";
>>> +  else
>>> +    return "__llvm_pgo_data";
>>> +}
>> 
>> This smells wrong to me. (1) We're at a pretty high level for checking the
>> triple, and (2) why is MachO special?
>
> In MachO, you need to specify a segment and a section.  Not true for ELF
> (I admit I don’t know anything about COFF, or much at all about linkage).
>
> Since we want symbols to go to a special section, we need to know which
> triple so we can format the section name correctly.

Gross. I'd really like this to go away in the future, but it looks to me
like this is the best we can do for now.

>>>> void CodeGenPGO::setFuncName(llvm::Function *Fn) {
>>>> -  StringRef Func = Fn->getName();
>>>> +  NormalFuncName = Fn->getName();
>>> 
>>> NormalFuncName? Is there a SpecialFuncName? This is just FunctionName,
>>> isn't it?
>> 
>> There are three function names kicking around there:
>> 
>> - Fn->getName(), which sometimes has a \01 prefix.
>> 
>> - NormalFuncName, which takes off the \01 prefix.  It’s used as a suffix
>>   for symbol names, like the “foo” in __llvm_pgo_counts_foo.
>> 
>> - FuncName, which already existed, and whose name I didn’t want to
>>   change, and for static variables has a filename prefix.  E.g.,
>>   “somefile.cpp:foo" if foo() is static and defined in somefile.cpp, or
>>   “bar" if bar() is not static.  This is what gets saved in
>>   __llvm_pgo_name_foo (or __llvm_pgo_name_bar) and used to be sent as an
>>   argument into llvm_pgo_writeout().
>> 
>> NormalFuncName is normal in that it has no \01 or filename prefix.
>> 
>> I’m open to suggestions, but FunctionName would clash awkwardly with
>> FuncName.
>
> I renamed both.
>
>   - FuncName => PrefixedFuncName
>   - NormalFuncName => RawFuncName
>
> I don’t like either name, since Prefixed isn’t always prefixed and
> Raw seems to indicate (incorrectly) that it includes the \01, but I
> think it’s an improvement.  Let me know if you come up with
> something better.

I think RawFuncName fits pretty well. Prefixed isn't great, but I don't
have much of a better suggestion (DecoratedFuncName? PGOVarFuncName?
InstrumentationVariableFuncName?). The names you went with should be
clear enough.

> @@ -77,7 +79,11 @@ public:
>  
>    /// Get the string used to identify this function in the profile data.
>    /// For functions with local linkage, this includes the main file name.
> -  const StringRef getFuncName() const { return StringRef(*FuncName); }
> +  StringRef getFuncName() const { return StringRef(*PrefixedFuncName); }
> +  std::string getFuncVarName(StringRef VarName) const {
> +    return Twine("__llvm_pgo_").concat(VarName).concat("_")
> +      .concat(RawFuncName).str();
> +  }

Using operator+ might be clearer for this.

> +typedef struct __llvm_pgo_data {
> +  const uint32_t NameSize;
> +  const uint32_t NumCounters;
> +  const char *const Name;
> +  const uint64_t *const Counters;
> +} DataType;

It's odd that the name of this struct is nothing like the typedef. Also,
DataType is a pretty bad name. "struct ProfileData" is a pretty good
name, which you could then typedef to ProfileData if you like.