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

[Eli Friedman]

On Mon, Sep 19, 2011 at 2:27 PM, Guo, Xiaoyi <Xiaoyi.Guo at amd.com> wrote:
> Thanks for reviewing.
>
> The code you mentioned will eventually fall through to the code below and do the isa<Constant> check there:
>
> +        if (A == tmpOp0 && !isa<Constant>(A)) // A&(A^B) -> A & ~B
> +          return BinaryOperator::CreateAnd(A, Builder->CreateNot(B, "tmp"));

Err, right, nevermind... I was thinking of a case which doesn't actually exist.

Would you like me to commit this?

-Eli

> Xiaoyi
>
> -----Original Message-----
> From: Eli Friedman [mailto:eli.friedman at gmail.com]
> Sent: Monday, September 19, 2011 2:21 PM
> To: Guo, Xiaoyi
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] bug fix for infinite loop in InstCombine
>
> On Mon, Sep 19, 2011 at 2:12 PM, Guo, Xiaoyi <Xiaoyi.Guo at amd.com> wrote:
>> Ping?
>
> Oh, hmm... I somehow thought I already had reviewed this.
>
> +    {
> +      Value *tmpOp0 = Op0;
> +      Value *tmpOp1 = Op1;
> +      if (Op0->hasOneUse() &&
> +          match(Op0, m_Xor(m_Value(A), m_Value(B)))) {
> +        if (A == Op1 || B == Op1 ) {
> +          tmpOp1 = Op0;
> +          tmpOp0 = Op1;
> +          // Simplify below
> +        }
>
> It looks like a check for isa<Constant>(Op1) check is necessary to handle some cases?
>
> Otherwise, looks fine.
>
> -Eli
>
>> -----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
>>
>>
>>
>
>
>