[llvm-commits] [PATCH] Implement a few missing InstCombine/InstSimplify optimizations

[David Majnemer]

Hi Duncan,

On Tue, May 31, 2011 at 2:28 AM, Duncan Sands <baldrick at free.fr> wrote:
> Hi David,
>
>> This patch implements a few of the optimizations mentioned in
>> http://www.nondot.org/sabre/LLVMNotes/InstCombine.txt
>>
>> specifically:
>> if ((x&  C) == 0) x |= C      becomes         x |= C
>> if ((x&  C) == 0) x&= ~C      becomes         nothing
>> if (((1<<  which_alternative)&  0x7)) becomes if (which_alternative<  3)
>> if (!((1<<  which_alternative)&  0x3)) becomes if (which_alternative>= 2)
>>
>> The patch also implements slightly more powerful variations of the
>> first two, C need not be a constant.
>
> unfortunately the first one is wrong if C is not a "single bit" (power of 2).
> Consider for example
>   if ((x & C) == 0) x |= C
> Take x = 2 and C = 3.  Then x&C=2, so (x&C)==0 is false.  Thus the result is
> the original value of x, namely 2.  You propose replacing x with x|C in this
> case which is equal to 3.  So this changes the result from 2 to 3, which is
> wrong.
You are correct, I somehow transformed this in my head to if ((x & C)
!= C) x |= C, the transform must be more strict.

>
> Also, in this bit
>
> +    ConstantInt *AndCI, *And2CI;
> +    // Transform: "if ((x & C) == 0) x &= ~C" ==> x &= ~C
> +    if (match(AndRHS, m_ConstantInt(AndCI)) &&
> +        match(TrueVal, m_And(m_Specific(FalseVal), m_ConstantInt(And2CI))) &&
> +        AndCI == ConstantExpr::getNot(And2CI)) {
> +      return FalseVal;
> +    }
> +    // Transform: "if ((x & y) == 0) x &= ~y" ==> x &= ~y
> +    if (match(TrueVal, m_And(m_Specific(FalseVal), m_Not(m_Specific(AndRHS))))) {
> +      return FalseVal;
> +    }
>
> the first part should be redundant, i.e. caught by the second part.  If it is
> not then I think you would do better to enhance m_Not to make the second part
> catch everything.
Just realized that those comments are wrong, it should be (x & y) != 0....

Yes, I also assumed that m_Not would match ConstantInt, however it
does not. I considering making the change you recommended but decided
that m_Not is used in quite a few places and changing the semantics of
m_Not in this way would slow it down and potentially break things. I
am guessing that most uses of m_Not that would hit ConstantInt would
be smashed away by constant folding instead of being this case... With
that in mind I am open to modifying m_Not, I just don't know how much
the consumers of that match would like it.

With that in mind "if ((x & y) != 0) x &= ~y" ==> "x &= ~y" is wrong
while "if ((x & y) == 0) x &= ~y" ==> "x" is OK

>
> That said, I don't much like these changes to InstructionSimplify.  What LLVM
> needs in general and InstructionSimplify in particular is a generic machinery
> for reasoning about simple implications.  Consider "select Cond, TV, FV".  If
> you can prove the implication "Cond => (TV == FV)" then you can replace the
> select with FV.  Your code above is a special case of this.  I would rather
> see a more general infrastructure for proving implications.
Hrm, that does sound useful but it also seems like a serious reworking
of InstructionSimplify...
>
> Ciao, Duncan.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>