<div dir="ltr">Ah. That's reasonable. I suspect there's no actual changes to the output, but I'll go check that. Thanks for the clarification.</div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Apr 4, 2017 at 11:09 AM, Renato Golin via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">rengolin added a comment.<br>
<span class=""><br>
In <a href="https://reviews.llvm.org/D31625#718083" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D31625#718083</a>, @niravd wrote:<br>
<br>
> This is specifically what Chandler requested on a similar previous patch (see <a href="https://reviews.llvm.org/D31286" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D31286</a>).<br>
<br>
<br>
</span>What Chandler requested in that commit is what James is requesting now. A meaningful check on the generated instructions.<br>
<br>
Not nothing. Not everything. A meaningful representation of the problem, which normally translates to a short (3-5) sequence of instructions that change between bugged and non-bugged versions.<br>
<br>
Both Chandler and James are absolutely correct that checking for failure as well as checking for the exact sequence will generate problems later and it's not acceptable.<br>
<br>
If you don't know what to check for, run the program before and after your patch and see what changes. This will also give confidence to the reviewers that your patch is only changing what's required and needed, not some random parts of the program.<br>
<br>
cheers,<br>
--renato<br>
<br>
<br>
<a href="https://reviews.llvm.org/D31625" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D31625</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>