<div dir="ltr">On Fri, Nov 8, 2013 at 2:57 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="im">On 5 November 2013 17:44, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><div class="im">

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>On 5 November 2013 17:30, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br>


</div><div class="gmail_extra"><div class="gmail_quote"><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">The test passes even with the code change reverted.<br></blockquote>



<div><br></div></div><div>Bah! Thanks for noticing. The testcase worked in C++ mode but not C mode.</div><div><br></div><div>Adding it to test/CodeGenCXX/catch-undef-behavior.cpp doesn't work well either because of ordering dependencies in that test.</div>



<div><br></div><div>Updated patch attached!</div></div></div></div></blockquote><div><br></div></div><div>In person at the llvm dev mtg, Richard LG'd this patch with comments. I wanted to make sure those comments are on the mailing list.</div>


<div><br></div><div>Just because Evaluate returns true doesn't mean that IRGen will emit a constant. It means that it's constant evaluable and that the optimizers could form a constant out of it. Of course, what is a constant anyways, many llvm constants lower to multiple instructions.</div>


<div><br></div><div>So assuming that we want to have this method at all, this is okay as an incremental patch but it's not particularly satisfying. We'd rather have IRGen detect whether it's an AST that would actually lower to a constant.</div>
</div></div></div></blockquote><div><br></div><div>Yep. I'm happy for this to go in as-is, but what I think we really want here is:</div><div><br></div><div>Try to emit the LHS and RHS as constants. If you succeed, emit a select, otherwise emit a branch to evaluate the operand(s) that we couldn't emit as constants.</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 class="im"><div>Nick</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><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><div>On 5 November 2013 16:54, Nick Lewycky <<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>> wrote:<br>



> When lowering "cond ? X : Y" we do some safety checks to see whether we can<br>
> instead emit both X and Y and use a llvm select instruction to choose<br>
> between them. This code is insufficiently safe, and introducing loads into a<br>
> program that didn't load is a bad idea. For example, it could be TLS. It<br>
> could be a non-volatile auto in another function that isn't the current<br>
> function (think lambdas).<br>
><br>
> Don't do this here. LLVM knows how to do this properly.<br>
><br>
> Patch attached, please review!<br>
><br>
> Nick<span style="color:rgb(34,34,34)"> </span></div></div></blockquote></div></div></div></div></div></blockquote></div></div></div></div>
<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div></div>