[llvm-commits] [PATCH] Bug 14664: InstCombine missing canonicalization of sub-and into a select

[Duncan Sands]

Hi Muhammad,

On 21/01/13 18:36, Ahmad, Muhammad T wrote:
> Makes sense.
>
> Please find the attached updated diff.

looks great to me, thanks for doing this.  Please add a test for the sext ->
zext transform and commit it.

>
> (not sure if I need to post all this to llvm-commits list)

You are right to CC llvm-commits.

Ciao, Duncan.

>
> Thanks.
>
> - Muhammad Tauqir
> ________________________________________
> From: Duncan Sands [duncan.sands at gmail.com] on behalf of Duncan Sands [baldrick at free.fr]
> Sent: Monday, January 21, 2013 9:01 AM
> To: Ahmad, Muhammad T
> Subject: Re: [llvm-commits] [PATCH] Bug 14664: InstCombine missing canonicalization of sub-and into a select
>
> Hi Muhammad Tauqir,
>
> On 21/01/13 17:53, Ahmad, Muhammad T wrote:
>> Thanks for the detailed feedback. It really helps.
>>
>> Some observations:
>> 1. I think we still need the 'match' method to get the value of ZExt operand into X.
>> 2. We would need X->getType()->isIntegerTy(1) instead of just X->isIntegerTy(1).
>
> you are quite right, sorry for the silly typos.  The patch looks good to me.
> However, as the following transform is true too:
>     (sub 0, (sext bool to B)) --> (zext bool to B)
> (i.e. "sub 0, extension" swaps sext and zext) can you please add that transform
> also.  You could try to handle both cases in the same code block (and if you can
> find a clever way of doing that then why not?) but I think you should just
> repeat the whole thing only with sext and zext swapped in the second one.
>
> Ciao, Duncan.
>
>>
>> Please find the attached updated diff.
>>
>> Thanks.
>>
>> - Muhammad Tauqir
>> ________________________________________
>> From: Duncan Sands [duncan.sands at gmail.com] on behalf of Duncan Sands [baldrick at free.fr]
>> Sent: Saturday, January 19, 2013 2:49 AM
>> To: Ahmad, Muhammad T
>> Cc: llvm-commits at cs.uiuc.edu
>> Subject: Re: [llvm-commits] [PATCH] Bug 14664: InstCombine missing canonicalization of sub-and into a select
>>
>> Hi Ahmed,
>>
>> On 19/01/13 00:09, Ahmad, Muhammad T wrote:
>>> Thanks Duncan for the feedback.
>>>
>>> Please find the updated diff.
>>> Instructions of form '(sub 0 (zext(bool)))' --> 'sext(bool)' now.
>>
>> thanks!  See below for comments.
>>
>>>
>>> I guess a separate discussion would then address '(and (sext(bool)), X)' --> '(select bool, X, 0)'?
>>
>> Exactly.
>>
>>> diff --git lib/Transforms/InstCombine/InstCombineAddSub.cpp lib/Transforms/InstCombine/InstCombineAddSub.cpp
>>> index 03be8ef..58a6eea 100644
>>> --- lib/Transforms/InstCombine/InstCombineAddSub.cpp
>>> +++ lib/Transforms/InstCombine/InstCombineAddSub.cpp
>>> @@ -1250,6 +1250,16 @@ Instruction *InstCombiner::visitSub(BinaryOperator &I) {
>>>
>>>        if (SimplifyDemandedInstructionBits(I))
>>>          return &I;
>>> +
>>> +    // Fold (sub 0, (zext A to B)) --> (sext A to B)
>>> +    // where type of A is i1
>>
>> This comment could all be on one line without exceeding 80 columns I think.
>>
>>> +    if (C->isZero() &&
>>> +        match(Op1, m_ZExt(m_Value()))) {
>>
>> This could also be one line.
>> Here I think you should declare a helper Value *X, and do:
>>
>>      if (C->isZero() && m_ZExt(m_Value(X))) {
>>
>> Then the operand of the zext would go in X.
>>
>>> +      CastInst *zextI = dyn_cast<CastInst>(Op1);
>>
>> This would not then be needed.  In any case you should use cast instead of
>> dyn_cast if you are sure a cast will succeed.  If Op1 is not castable to
>> a CastInst then "cast" will assert, while "dyn_cast" will return null and
>> you will get a less understandable crash on the following line:
>>
>>> +      if (zextI->getSrcTy()->isIntegerTy(1))
>>
>> Here you could then just do:
>>
>>      if (X->isIntegerTy(1))
>>
>>> +        return CastInst::CreateSExtOrBitCast(zextI->getOperand(0),
>>> +                                             zextI->getDestTy());
>>
>> and this would become
>>
>>      return CastInst::CreateSExtOrBitCast(X, Op1->getType());
>>
>>> +    }
>>>      }
>>>
>>
>> Ciao, Duncan.
>>
>