<div dir="ltr">I posted the revised patch in the old review link:<div><a href="http://reviews.llvm.org/D12781">http://reviews.llvm.org/D12781</a><br></div><div><br></div><div>It's still marked as closed. Let me know if you think I need to open another review.</div><div><br></div><div>Testing is still ongoing. No issues so far.</div><div><br></div><div>Thanks,</div><div><br></div><div>-Rong</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 25, 2015 at 12:21 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Tue, Nov 24, 2015 at 8:50 PM, Rong Xu <span dir="ltr"><<a href="mailto:xur@google.com" target="_blank">xur@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Tue, Nov 24, 2015 at 4:46 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Tue, Nov 24, 2015 at 4:20 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Tue, Nov 24, 2015 at 4:07 PM, Rong Xu <span dir="ltr"><<a href="mailto:xur@google.com" target="_blank">xur@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"><div dir="ltr"><div>Hi Sean,</div><div><br></div>Sorry about this. I revert the change a moment ago.<div><br></div><div>As for the test. Here is the way I want to fix:</div><div><div>In profile-generate tests, I will test (1) number of counters inserted in each function, and (2) If there is a instrumentation in a specific bb. I will skip the count index.</div></div><div><br></div><div><div>In profile-use, I will invoke a new pass branch-prob that computes the branch probability. I will check the branch probability rather the branch weight metadata.</div></div><div><br></div><div>Let me know if you think this is reasonable, or you have a better way to do the test.</div></div></blockquote><div><br></div></span><div>You can use FileCheck variable captures (<a href="http://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-variables" target="_blank">http://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-variables</a>) to match up the indexes (see the example where it captures a register assignment and checks that a later instruction uses that register). Together with good use of CHECK lines, you should be able to capture exactly what you need and make the test very readable.</div><div><br></div><div>The profile-use pass only adds metadata, so we should be checking the metadata. We should not need to invoke branch-prob to test the behavior.</div><div><br></div><div>Also, Hal's point is worth investigating: why is this pass not deterministic across platforms? LLVM code should have deterministic output independent of the host platform.</div></div></div></div></blockquote><div><br></div></span><div>One thing I noticed is that std::stable_sort should be probably be used instead of std::sort.</div></div></div></div></blockquote><div><br></div></span><div>It seems all the test failures are caused by std::sort. </div><div>I used darwin to verify. Once using stable_sort, the instrumentation order became</div><div>identical in two platforms and all the test turns green.</div></div></div></div></blockquote><div><br></div></span><div>Great, could you open up the review again with the revised patch? (or create a new review if Phab does not support reopening)</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>-- Sean Silva</div></font></span><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><font color="#888888"><div><br></div><div>-Rong</div></font></span><div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>Another thing is that the dumping traverses a map indexed by address -- this can result in different order on different host -- but the test cases do not depend on that (debug dump).</div><span><font color="#888888"><div><br></div><div>david</div></font></span><div><div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><font color="#888888"><div><br></div><div>-- Sean Silva</div></font></span><div><div><div> </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"><div dir="ltr"><div><br></div><div>Thanks,</div><div><br></div><div>-Rong</div><div><br></div><div><br></div></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 24, 2015 at 3:16 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.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"><div dir="ltr">Hi Rong,<div><br></div><div>Typically, when there have been multiple active reviewers, it is common courtesy to wait for all of them to LGTM the patch. </div><div><br></div><div>I had further comments on the testing here (which the bots seem to have caught you on anyway), so I recommend reverting this patch for the moment and reopening the review.</div><span><font color="#888888"><div><br></div><div>-- Sean Silva</div><div><br></div></font></span></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 24, 2015 at 1:34 PM, Rong Xu <span dir="ltr"><<a href="mailto:xur@google.com" target="_blank">xur@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">This revision was automatically updated to reflect the committed changes.<br>
Closed by commit rL254021: [PGO] MST based PGO instrumentation infrastructure (authored by xur).<br>
<br>
Changed prior to commit:<br>
<a href="http://reviews.llvm.org/D12781?vs=41062&id=41079#toc" rel="noreferrer" target="_blank">http://reviews.llvm.org/D12781?vs=41062&id=41079#toc</a><br>
<br>
Repository:<br>
rL LLVM<br>
<br>
<a href="http://reviews.llvm.org/D12781" rel="noreferrer" target="_blank">http://reviews.llvm.org/D12781</a><br>
<br>
Files:<br>
llvm/trunk/include/llvm/IR/DiagnosticInfo.h<br>
llvm/trunk/include/llvm/InitializePasses.h<br>
llvm/trunk/include/llvm/LinkAllPasses.h<br>
llvm/trunk/include/llvm/Transforms/Instrumentation.h<br>
llvm/trunk/lib/IR/DiagnosticInfo.cpp<br>
llvm/trunk/lib/Transforms/IPO/LLVMBuild.txt<br>
llvm/trunk/lib/Transforms/Instrumentation/CFGMST.h<br>
llvm/trunk/lib/Transforms/Instrumentation/CMakeLists.txt<br>
llvm/trunk/lib/Transforms/Instrumentation/Instrumentation.cpp<br>
llvm/trunk/lib/Transforms/Instrumentation/LLVMBuild.txt<br>
llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp<br>
llvm/trunk/test/Transforms/PGOProfile/Inputs/branch1.proftext<br>
llvm/trunk/test/Transforms/PGOProfile/Inputs/branch2.proftext<br>
llvm/trunk/test/Transforms/PGOProfile/Inputs/criticaledge.proftext<br>
llvm/trunk/test/Transforms/PGOProfile/Inputs/landingpad.proftext<br>
llvm/trunk/test/Transforms/PGOProfile/Inputs/loop1.proftext<br>
llvm/trunk/test/Transforms/PGOProfile/Inputs/loop2.proftext<br>
llvm/trunk/test/Transforms/PGOProfile/Inputs/loop3.proftext<br>
llvm/trunk/test/Transforms/PGOProfile/Inputs/single_bb.proftext<br>
llvm/trunk/test/Transforms/PGOProfile/Inputs/switch.proftext<br>
llvm/trunk/test/Transforms/PGOProfile/branch1_gen.ll<br>
llvm/trunk/test/Transforms/PGOProfile/branch1_use.ll<br>
llvm/trunk/test/Transforms/PGOProfile/branch2_gen.ll<br>
llvm/trunk/test/Transforms/PGOProfile/branch2_use.ll<br>
llvm/trunk/test/Transforms/PGOProfile/checksum_mismatch.ll<br>
llvm/trunk/test/Transforms/PGOProfile/criticaledge_gen.ll<br>
llvm/trunk/test/Transforms/PGOProfile/criticaledge_use.ll<br>
llvm/trunk/test/Transforms/PGOProfile/landingpad_gen.ll<br>
llvm/trunk/test/Transforms/PGOProfile/landingpad_use.ll<br>
llvm/trunk/test/Transforms/PGOProfile/loop1_gen.ll<br>
llvm/trunk/test/Transforms/PGOProfile/loop1_use.ll<br>
llvm/trunk/test/Transforms/PGOProfile/loop2_gen.ll<br>
llvm/trunk/test/Transforms/PGOProfile/loop2_use.ll<br>
llvm/trunk/test/Transforms/PGOProfile/loop3_gen.ll<br>
llvm/trunk/test/Transforms/PGOProfile/loop3_use.ll<br>
llvm/trunk/test/Transforms/PGOProfile/noprofile_use.ll<br>
llvm/trunk/test/Transforms/PGOProfile/single_bb_gen.ll<br>
llvm/trunk/test/Transforms/PGOProfile/switch_gen.ll<br>
llvm/trunk/test/Transforms/PGOProfile/switch_use.ll<br>
<br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>