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

[Nick Lewycky]

Jay Foad wrote:
>> 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.

Ok, Op1CI then.

>
>> +    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?

Yep.

  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?

That's a great point.

We don't want to put ourselves in a situation where we do an iteration 
over every instruction calling a method that does a walk over all uses. 
For an n-instruction long chain of computation, that would give us 
O(n^2) time.

ComputeMaskedBits has a partner in crime, SimplifyDemandedBits which is 
used by the instruction combiner. SimplifyDemandedBits is similar to 
ComputeMaskedBits but also modifies a chain of computation that touches 
irrelevant (masked) bits. Its best trick is deleting irrelevant 
and/xor/or instructions. I suppose it could also try to replace a more 
complex instruction with a simpler one (add -> or) knowing that the bits 
which come out different are going to be ignored anyway.

This time, I left SimplifyDemandedBits alone, because for accuracy in 
ComputeMaskedBits the new mask is actually all-one bits, which is the 
most useless possible mask for SimplifyDemandedBits.

But perhaps I should revisit that and see if we can't come up with a way 
to calculate the "demanded" bits in an add, based on the known bits on 
the lhs and rhs and the mask of which bits of output are relevant.

Nick