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

[Guo, Xiaoyi]

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"));

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