On Mon, Dec 14, 2015 at 3:26 PM, Xinliang David Li <davi...@google.com> wrote:
> On Fri, Dec 11, 2015 at 6:19 PM, Justin Bogner <m...@justinbogner.com> > wrote: > > Easwaran Raman <era...@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(); > >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
