<div dir="ltr">Point taken :)<div><br></div><div>1) The test cases need to be greatly simplified -- same control flow with original tests but with bare minimal other code</div><div>2) use text profile data.</div><div><br></div><div>thanks,</div><div><br></div><div>David</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Nov 20, 2015 at 1:07 PM, Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Sean Silva <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>> writes:<br>
> These test files look like they are just a dump of IR generated from a<br>
> C/C++, which is extremely verbose and has a lot of inessential<br>
> details. I also don't like checking in binary profdata files. Overall<br>
> these tests seem extremely brittle. I also don't understand why the<br>
> test files have names like "for" or "goto" or "ifelse". Those concepts<br>
> don't exist in the IR. Surely the tests should have names like<br>
> "criticaledge" etc.<br>
><br>
> It seems like this testing might be more readable as C++ unit<br>
> tests. Using the new pass manager this should be easy to wire up. I<br>
> think the hardest part would be to stub out IndexedInstrProfReader.<br>
<br>
</span>Unit tests might be appropriate, but even as lit tests these can and<br>
should be massively simplified. The tests here do a lot of work that<br>
isn't relevant to what's being tested - there are loads and stores to<br>
various things which really have to do with the profile metadata.<br>
<br>
Hand rolled IR that is representative of interesting LLVM IR contructs<br>
would be much clearer about what's being tested, since it wouldn't have<br>
irrelevant details. This combined with using the .proftext format so<br>
that the input profiles are human readable and editable should simplify<br>
these tests greatly, without losing any coverage of the feature.<br>
</blockquote></div><br></div>