<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">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><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><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>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><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><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"><span class=""><font color="#888888"><div><br></div><div>David</div><div><br></div><div><br></div></font></span></div><div class=""><div class="h5"><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></div><br></div></div>