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

[Redmond, Paul]

Committed in r173093

On 2013-01-21, at 3:22 PM, Duncan Sands wrote:

> 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.
>>> 
>> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits