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

[Duncan Sands]

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.