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

[Ahmad, Muhammad T]

Makes sense.

Please find the attached updated diff.

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

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.
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: zext_sub_to_sext_2013-01-21.diff
Type: text/x-patch
Size: 2042 bytes
Desc: zext_sub_to_sext_2013-01-21.diff
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130121/c83129a5/attachment.bin>