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

[Guo, Xiaoyi]

Thank you!

-----Original Message-----
From: Eli Friedman [mailto:eli.friedman at gmail.com] 
Sent: Monday, September 19, 2011 3:00 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:49 PM, Guo, Xiaoyi <Xiaoyi.Guo at amd.com> wrote:
>> Would you like me to commit this?
>
> I would appreciate that.

r140072.

-Eli

> Thanks,
> Xiaoyi
>
> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu 
> [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Eli Friedman
> Sent: Monday, September 19, 2011 2:37 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: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
>>>
>>>
>>>
>>
>>
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>