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

[Justin Bogner via cfe-commits]

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

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