[PATCH] D15163: Attach maximum function count to Module when using PGO mode.

[Xinliang David Li via cfe-commits]

On Fri, Dec 11, 2015 at 6:19 PM, Justin Bogner <mail at justinbogner.com> wrote:
> Easwaran Raman <eraman at google.com> writes:
>> eraman updated this revision to Diff 42549.
>> eraman added a comment.
>>
>> Added a test case.
>>
>>
>> Repository:
>>   rL LLVM
>>
>> http://reviews.llvm.org/D15163
>>
>> Files:
>>   lib/CodeGen/CodeGenModule.cpp
>>   test/CodeGen/pgo-max-function-count.c
>>
>> Index: test/CodeGen/pgo-max-function-count.c
>> ===================================================================
>> --- /dev/null
>> +++ test/CodeGen/pgo-max-function-count.c
>> @@ -0,0 +1,9 @@
>> +// RUN: %clang -fprofile-generate -o %t -O2 %s
>> +// RUN: env LLVM_PROFILE_FILE=%t.profraw  %t
>
> The clang tests are run in environments where the generated binaries
> can't run on the host system, so you can't do this. Instead of this kind
> of full integration test you should provide handwritten profile data to
> feed into the test. See the tests in test/Profile for examples, and also
> consider putting this test in that directory with the others.
>
>> +// RUN: llvm-profdata merge -o %t.profdata %t.profraw
>> +// RUN: %clang -fprofile-use=%t.profdata -o - -S -emit-llvm %s | FileCheck %s
>> +// Check
>> +int main() {
>> +  return 0;
>> +}
>> +// CHECK: !{{[0-9]+}} = !{i32 1, !"MaxFunctionCount", i32 1}
>> Index: lib/CodeGen/CodeGenModule.cpp
>> ===================================================================
>> --- lib/CodeGen/CodeGenModule.cpp
>> +++ lib/CodeGen/CodeGenModule.cpp
>> @@ -376,6 +376,9 @@
>>    }
>>    if (PGOReader && PGOStats.hasDiagnostics())
>>      PGOStats.reportDiagnostics(getDiags(), getCodeGenOpts().MainFileName);
>> +  // In PGO mode, attach maximum function count to the module.
>
> This comment isn't helpful - it's just stating exactly what the code
> that follows does.
>
>> +  if (PGOReader)
>> +    getModule().setMaximumFunctionCount(PGOReader->getMaximumFunctionCount());
>
> It would read better to fold the two `if (PGOReader)` checks
> together. ie:
>
>   if (PGOReader) {
>     getModule().setMaximumFunctionCount(PGOReader->getMaximumFunctionCount());
>     if (PGOStats.hasDiagnostics())
>       PGOStats.reportDiagnostics(getDiags(), getCodeGenOpts().MainFileName);
>   }
>
> That said, wouldn't it make more sense to set this within PGOReader
> itself? It feels pretty awkward for this to be happening in
> CodeGenModule::Release().

The reader does not know about module context. I think it is better to
put this in CodeGenModule constructor when PGOReader is created.

David


>
>>    EmitCtorList(GlobalCtors, "llvm.global_ctors");
>>    EmitCtorList(GlobalDtors, "llvm.global_dtors");
>>    EmitGlobalAnnotations();
>>