PATCH: do ?: lowering to selects less wrong

[Richard Smith]

On Fri, Nov 8, 2013 at 2:57 PM, Nick Lewycky <nlewycky at google.com> wrote:

> On 5 November 2013 17:44, Nick Lewycky <nlewycky at google.com> wrote:
>
>> On 5 November 2013 17:30, Rafael Espíndola <rafael.espindola at gmail.com>wrote:
>>
>>> The test passes even with the code change reverted.
>>>
>>
>> Bah! Thanks for noticing. The testcase worked in C++ mode but not C mode.
>>
>> Adding it to test/CodeGenCXX/catch-undef-behavior.cpp doesn't work well
>> either because of ordering dependencies in that test.
>>
>> Updated patch attached!
>>
>
> 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.
>
> 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.
>
> 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.
>

Yep. I'm happy for this to go in as-is, but what I think we really want
here is:

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.


> Nick
>
>
>> On 5 November 2013 16:54, Nick Lewycky <nlewycky at google.com> wrote:
>>> > When lowering "cond ? X : Y" we do some safety checks to see whether
>>> we can
>>> > instead emit both X and Y and use a llvm select instruction to choose
>>> > between them. This code is insufficiently safe, and introducing loads
>>> into a
>>> > program that didn't load is a bad idea. For example, it could be TLS.
>>> It
>>> > could be a non-volatile auto in another function that isn't the current
>>> > function (think lambdas).
>>> >
>>> > Don't do this here. LLVM knows how to do this properly.
>>> >
>>> > Patch attached, please review!
>>> >
>>> > Nick
>>>
>>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131108/8967dc5d/attachment.html>