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