[PATCH] Transform C - zext(bool) -> bool ? C - 1 : C

[Stephen Lin]

Hi David, Nick,

I don't have a problem with the patch either (I think the latter form
is more clearly canonical, I think), but I would just like to mention
that this is tangentially related to something I would like to do,
too:

    http://comments.gmane.org/gmane.comp.compilers.llvm.cvs/152547

Basically, the question is whether or not the code generators prefer
having selects of binops or binops of selects, when the two are
equivalent. Most binops are canonicalized to the latter already, so I
would like to fill in some gaps that I noticed. This would yield code
of the same basic form as your proposed canonicalization (a select
with a binop as an operand) and I would like to know if there are any
backends that generate suboptimal code from this form, in general.

It seems like the particular code lowering issue mentioned in the
previous commit is specific to the boolean case so (as far I can tell)
does not apply to the floating ops I would like to canonicalize.
However, if you do find or know of any other issue with backend
codegen of these patterns that is more generic, please let me know.

Thanks!
Stephen

On Fri, Jul 19, 2013 at 3:50 PM, Evan Cheng <evan.cheng at apple.com> wrote:
> I don't have a problem with the patch. But is there a follow up patch to fix
> the lowering problem?
>
> Looking back at the history. It mentioned this impacts a couple of small
> test cases:
>
> int
> blah(_Bool x, _Bool y) {
>   return (x - y) + 1;
> }
>
> int
> blah2(bool x) {
>   return x ? 0 : -1;
> }
>
> BTW, David filed the bug report that motivated the patch.
>
> Evan
>
> On Jul 19, 2013, at 2:10 PM, Nick Lewycky <nlewycky at google.com> wrote:
>
> Hi Evan, this is about your r159230,
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120625/145470.html
> . The canonicalization was added, then removed by you. In post-commit
> review, Eli asked you to pick one of the two forms to be canonical.
>
> I have no problem with David landing this patch as is -- it looks like
> goodness to me, particularly the "C - zext(X)" transform -- but I don't want
> to mess you up. Could you please chime in?
>
> Nick
>
>
> On 12 July 2013 16:03, David Majnemer <david.majnemer at gmail.com> wrote:
>>
>> The attached patch essentially reverts r159230.
>>
>> The reasons why are because:
>> 1. It creates an asymmetry in the IR by hurting our ability to
>> canonicalize things like C - zext(X) and C + zext(!X) into the same thing.
>> 2. Adding it back does not break any tests and without it we still
>> generate the inferior code mentioned in the commit log.
>>
>> It seems like we should keep the canonicalization and fix (the apparently
>> still broken problem) in the lowering stage.
>> The commit mentioned that some targets don't have a sext i1. This
>> shouldn't be a concern at the IR level, should it?
>>
>> --
>> David Majnemer
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>