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

[Easwaran Raman via cfe-commits]

On Mon, Dec 14, 2015 at 3:26 PM, Xinliang David Li <davidxl at google.com>
wrote:

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

The reason I chose to do it here is because  CodeGenModule::Release() is
where other module flags get added.

>
> David
>
>
> >
> >>    EmitCtorList(GlobalCtors, "llvm.global_ctors");
> >>    EmitCtorList(GlobalDtors, "llvm.global_dtors");
> >>    EmitGlobalAnnotations();
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151214/2f17999d/attachment.html>