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

[Ahmad, Muhammad T]

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

Please find the attached updated diff.

Thanks.

(sorry for the spam Duncan)

- 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: 1817 bytes
Desc: zext_sub_to_sext_2013-01-21.diff
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130121/87d7c2e3/attachment.bin>