<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 19, 2015 at 10:00 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"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Thu, Nov 19, 2015 at 7:22 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"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Thu, Nov 19, 2015 at 6:59 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"><div dir="ltr">I thought about the size of the test case too. Initially I had the same concern, but in second look, it does not seem to be brittle -- because it just checks whether the key branches are properly annotated with the right profile count or not, or profile counter update instruction is inserted or not.</div></blockquote><div><br></div></span><div>It is brittle because it depends on checked in binary files that need to be regenerated. For example, a person on Mac or Windows might have trouble regenerating IR with `target triple = "x86_64-unknown-linux-gnu"`...</div><span><div> </div></span></div></div></div></blockquote><div><br></div></span><div>This is the current common practice in testing PGO use and coverage. There are existing binary profile data checked in for both Instrument and Sample PGO.  </div><div><br></div><div>Also I am not sure what you mean by re-generating IR -- there is no need to re-generate IR.  Indexed profile data needs to be generated occasionally, but their format is system independent.  For the same program with the same training set, the indexed profile should be identical regardless whether the running system is 32bit or 64bit, LE or BE.</div></div></div></div></blockquote><div><br></div><div>Sorry, I missed some words there, I meant something like "regenerating the profiles from IR with IR with `target triple = "x86_64-unknown-linux-gnu"`". For a binary profile file, that is probably an easier route than trying to edit the file directly.</div><div>Using the text format as Justin suggests will hopefully avoid some of these issues because text files are easier to edit/modify/understand.</div><div><br></div><div>-- Sean Silva</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 class="gmail_extra"><div class="gmail_quote"><span class=""><font color="#888888"><div><br></div><div>David</div></font></span><span class=""><div><br></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 class="gmail_extra"><div class="gmail_quote"><span><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>I think adding C++ unit tests are independent tasks which can be done once the driver part of the changes land.</div></div></blockquote><div><br></div></span><div>I assume by "driver" you mean the clang driver. Note that the IR level instrumentation must be independent of what frontend generated the IR, so I don't see the connection between the testing for this patch and any work in clang.</div><div><br></div><div>(FYI "unit tests" in LLVM-land means the C++ tests in unittests/; the rest are "lit tests"; I said "C++ unit tests" previously because it seemed that Rong was unfamiliar with the LLVM terminology and I didn't want to distract by clarifying this</div><div>)</div><span><font color="#888888"><div><br></div><div>-- Sean Silva</div></font></span><span><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"><span><font color="#888888"><div><br></div><div>David</div><div><br></div><div><br></div></font></span></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 19, 2015 at 6:40 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">silvas added a comment.<br>
<br>
These test files look like they are just a dump of IR generated from a C/C++, which is extremely verbose and has a lot of inessential details. I also don't like checking in binary profdata files. Overall these tests seem extremely brittle. I also don't understand why the test files have names like "for" or "goto" or "ifelse". Those concepts don't exist in the IR. Surely the tests should have names like "criticaledge" etc.<br>
<br>
It seems like this testing might be more readable as C++ unit tests. Using the new pass manager this should be easy to wire up. I think the hardest part would be to stub out IndexedInstrProfReader.<br>
<br>
<br>
<a href="http://reviews.llvm.org/D12781" rel="noreferrer" target="_blank">http://reviews.llvm.org/D12781</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>
</div></div></blockquote></span></div><br></div></div>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>