[llvm-commits] bug fix for infinite loop in InstCombine

[Guo, Xiaoyi]

Ping?

-----Original Message-----
From: Guo, Xiaoyi 
Sent: Thursday, September 15, 2011 6:34 PM
To: 'Eli Friedman'
Cc: llvm-commits at cs.uiuc.edu
Subject: RE: [llvm-commits] bug fix for infinite loop in InstCombine

Thanks for reviewing it. Here's another try. See attached new diff.

Xiaoyi

-----Original Message-----
From: Eli Friedman [mailto:eli.friedman at gmail.com]
Sent: Thursday, September 15, 2011 5:55 PM
To: Guo, Xiaoyi
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [llvm-commits] bug fix for infinite loop in InstCombine

On Thu, Sep 15, 2011 at 4:45 PM, Guo, Xiaoyi <Xiaoyi.Guo at amd.com> wrote:
> Please review the attached fix for a bug in InstCombiner. The patch also includes a new test case which will cause the current InstCombiner to go into an infinite loop.
>
> Please help to commit if acceptable.
>
> The problem is with the following code in InstCombiner::visitAnd() in InstCombineAndOrXor.cpp:
>
> Instruction *InstCombiner::visitAnd(BinaryOperator &I) {
>  bool Changed = SimplifyAssociativeOrCommutative(I);
>  ...
>    if (Op0->hasOneUse() &&
>        match(Op0, m_Xor(m_Value(A), m_Value(B)))) {
>      if (A == Op1) {                                // (A^B)&A ->
> A&(A^B)
>        I.swapOperands();     // Simplify below
>        std::swap(Op0, Op1);                            <==========
>      } else if (B == Op1) {                         // (A^B)&B ->
> B&(B^A)
>        cast<BinaryOperator>(Op0)->swapOperands();
>        I.swapOperands();     // Simplify below
>        std::swap(Op0, Op1);                            <==========
>      }
>    }
>
>    if (Op1->hasOneUse() &&
>        match(Op1, m_Xor(m_Value(A), m_Value(B)))) {
>      if (B == Op0) {                                // B&(A^B) ->
> B&(B^A)
>        cast<BinaryOperator>(Op1)->swapOperands();
>        std::swap(A, B);
>      }
>      // Notice that the patten (A&(~B)) is actually (A&(-1^B)), so if
>      // A is originally -1 (or a vector of -1 and undefs), then we 
> enter
>      // an endless loop. By checking that A is non-constant we ensure 
> that
>      // we will never get to the loop.
>      if (A == Op0 && !isa<Constant>(A)) // A&(A^B) -> A & ~B
>        return BinaryOperator::CreateAnd(A, Builder->CreateNot(B, 
> "tmp"));
>    }
>
>
> With the given test case, op0 is a const and op1 is xor, so the two operands are be swapped in SimplifyAssociativeOrCommutative(). Then I's operands are swapped back at one of the statements marked "<===========" above. Then because A is a constant, a new instruction is not created. So we end up with Changed flag as true but the instruction is not changed and we go into an infinite loop in InstCombiner::DoOneIteration().
>
> My fix is to remove the swapOperands() calls, because I don't think they are necessary.

The general idea seems fine.  Your fix disables the transform in some cases where both Op0 and Op1 are Xor operators, though.

-Eli