PATCH: do ?: lowering to selects less wrong
richard at metafoo.co.uk
Fri Nov 8 17:39:55 PST 2013
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
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
>> 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.
>>> > 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
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the cfe-commits