[llvm-commits] patch: go crazy, compute bits for an entire add instruction

[Jay Foad]

> Jay, would you be willing to review this updated patch, now for add+sub?

OK, first some comments on the and/or parts:

> Index: lib/Analysis/InstructionSimplify.cpp
> ===================================================================
> --- lib/Analysis/InstructionSimplify.cpp	(revision 135567)
> +++ lib/Analysis/InstructionSimplify.cpp	(working copy)
> @@ -1211,6 +1211,21 @@
>                                        MaxRecurse))
>        return V;
>
> +  // Check whether the and instruction can be solved through bitwise analysis.
> +  if (ConstantInt *CI = dyn_cast<ConstantInt>(Op1)) {

I'd find it easier to read if CI was renamed to C1, or CI1, or COp1,
or *anything* that makes it clear that it's Op1.

> +    unsigned BitWidth = cast<IntegerType>(Op0->getType())->getBitWidth();
> +    APInt KnownZero(BitWidth, 0), KnownOne(BitWidth, 0);
> +    ComputeMaskedBits(Op0, CI->getValue(), KnownZero, KnownOne, TD, 0);
> +
> +    // Are the bits being and'd away already known to be zero?
> +    if ((CI->getValue() | KnownZero).isMaxValue())
> +      return Op0;
> +
> +    // Are all of the bits left after masking known?
> +    if ((KnownZero | KnownOne) == CI->getValue())

Does ComputeMaskedBits guarantee not to return with any bits set in
KnownZero/KnownOne outside of the bits you specified in the mask? If
not (i.e. if it treats the mask just as an optimisation, which it can
ignore if it chooses) you want something like:

  if ((CI->getValue() & ~(KnownZero | KnownOne)) == 0)

> +      return ConstantInt::get(Op0->getType(), KnownOne);
> +  }
> +
>    return 0;
>  }
>
> @@ -1305,6 +1320,23 @@
>                                        MaxRecurse))
>        return V;
>
> +  // Check whether the or instruction can be solved through bitwise analysis.
> +  if (ConstantInt *CI = dyn_cast<ConstantInt>(Op1)) {
> +    unsigned BitWidth = cast<IntegerType>(Op0->getType())->getBitWidth();
> +    APInt KnownZero(BitWidth, 0), KnownOne(BitWidth, 0);
> +    ComputeMaskedBits(Op0, APInt::getAllOnesValue(BitWidth),
> +                      KnownZero, KnownOne, TD, 0);

Surely you can specify ~CI->getValue() as the mask? Maybe this is the
asymmetry that it sounds like Chris complained about.

> +
> +    // Are the bits being or'd in already known to be one?
> +    if ((CI->getValue() & KnownOne) == CI->getValue())
> +      return Op0;
> +
> +    // Are all of the unknown bits being set to true by the or?
> +    if ((KnownZero | KnownOne | CI->getValue()).isMaxValue())
> +      return ConstantInt::get(Op0->getType(),
> +                              (KnownOne | CI->getValue()));
> +  }
> +
>    return 0;
>  }


Overall I think this is all reasonably clear.

But, couldn't you easily extend it to cover the case where Op1 isn't a
constant? Something like:

// for AND
ComputeMaskedBits(Op1, AllOnes, Op1KnownZero, Op1KnownOne);
ComputeMaskedBits(Op0, ~Op1KnownZero, Op0KnownZero, Op0KnownOne);

ResKnownZero = Op0KnownZero | Op1KnownZero;
ResKnownOne = Op0KnownOne & Op1KnownOne;
if ((ResKnownZero | ResKnownOne).isMaxValue())
  // All result bits known
  return ConstantInt::get(Op0KnownOne & Op1KnownOne);

if ((Op0KnownZero | Op1KnownOne).isMaxValue())
  // Op0 won't be changed by ANDing with Op1
  return Op0;

if ((Op0KnownOne | Op1KnownZero).isMaxValue())
  // Op1 won't be changed by ANDing with Op0
  return Op1;

However, this duplicates some of the logic from ComputeMaskedBits()
itself: the case where we return a constant here is exactly the case
where, if we called ComputeMaskedBits on the result of the AND
operation, it would tell us that all the bits are known. So why are we
getting this duplication in the first place? Isn't there some common
place where we can call ComputeMaskedBits on every expression, and if
all the bits are known, replace the operation with a constant?

Jay.