<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Dec 14, 2015 at 3:26 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">On Fri, Dec 11, 2015 at 6:19 PM, Justin Bogner <<a href="mailto:mail@justinbogner.com">mail@justinbogner.com</a>> wrote:<br>
</span><div><div class="h5">> Easwaran Raman <<a href="mailto:eraman@google.com">eraman@google.com</a>> writes:<br>
>> eraman updated this revision to Diff 42549.<br>
>> eraman added a comment.<br>
>><br>
>> Added a test case.<br>
>><br>
>><br>
>> Repository:<br>
>>   rL LLVM<br>
>><br>
>> <a href="http://reviews.llvm.org/D15163" rel="noreferrer" target="_blank">http://reviews.llvm.org/D15163</a><br>
>><br>
>> Files:<br>
>>   lib/CodeGen/CodeGenModule.cpp<br>
>>   test/CodeGen/pgo-max-function-count.c<br>
>><br>
>> Index: test/CodeGen/pgo-max-function-count.c<br>
>> ===================================================================<br>
>> --- /dev/null<br>
>> +++ test/CodeGen/pgo-max-function-count.c<br>
>> @@ -0,0 +1,9 @@<br>
>> +// RUN: %clang -fprofile-generate -o %t -O2 %s<br>
>> +// RUN: env LLVM_PROFILE_FILE=%t.profraw  %t<br>
><br>
> The clang tests are run in environments where the generated binaries<br>
> can't run on the host system, so you can't do this. Instead of this kind<br>
> of full integration test you should provide handwritten profile data to<br>
> feed into the test. See the tests in test/Profile for examples, and also<br>
> consider putting this test in that directory with the others.<br>
><br>
>> +// RUN: llvm-profdata merge -o %t.profdata %t.profraw<br>
>> +// RUN: %clang -fprofile-use=%t.profdata -o - -S -emit-llvm %s | FileCheck %s<br>
>> +// Check<br>
>> +int main() {<br>
>> +  return 0;<br>
>> +}<br>
>> +// CHECK: !{{[0-9]+}} = !{i32 1, !"MaxFunctionCount", i32 1}<br>
>> Index: lib/CodeGen/CodeGenModule.cpp<br>
>> ===================================================================<br>
>> --- lib/CodeGen/CodeGenModule.cpp<br>
>> +++ lib/CodeGen/CodeGenModule.cpp<br>
>> @@ -376,6 +376,9 @@<br>
>>    }<br>
>>    if (PGOReader && PGOStats.hasDiagnostics())<br>
>>      PGOStats.reportDiagnostics(getDiags(), getCodeGenOpts().MainFileName);<br>
>> +  // In PGO mode, attach maximum function count to the module.<br>
><br>
> This comment isn't helpful - it's just stating exactly what the code<br>
> that follows does.<br>
><br>
>> +  if (PGOReader)<br>
>> +    getModule().setMaximumFunctionCount(PGOReader->getMaximumFunctionCount());<br>
><br>
> It would read better to fold the two `if (PGOReader)` checks<br>
> together. ie:<br>
><br>
>   if (PGOReader) {<br>
>     getModule().setMaximumFunctionCount(PGOReader->getMaximumFunctionCount());<br>
>     if (PGOStats.hasDiagnostics())<br>
>       PGOStats.reportDiagnostics(getDiags(), getCodeGenOpts().MainFileName);<br>
>   }<br>
><br>
> That said, wouldn't it make more sense to set this within PGOReader<br>
> itself? It feels pretty awkward for this to be happening in<br>
> CodeGenModule::Release().<br>
<br>
</div></div>The reader does not know about module context. I think it is better to<br>
put this in CodeGenModule constructor when PGOReader is created.<br></blockquote><div><br></div><div>The reason I chose to do it here is because  CodeGenModule::Release() is where other module flags get added. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><font color="#888888"><br>
David<br>
</font></span><div class=""><div class="h5"><br>
<br>
><br>
>>    EmitCtorList(GlobalCtors, "llvm.global_ctors");<br>
>>    EmitCtorList(GlobalDtors, "llvm.global_dtors");<br>
>>    EmitGlobalAnnotations();<br>
>><br>
</div></div></blockquote></div><br></div></div>