<br /><br /><span>On 08/20/15 01:01 PM, <b class="name">Justin Bogner </b> <mail@justinbogner.com> wrote:</span><blockquote cite="mid:m2pp2hk9v3.fsf@chronotis.apple.com" class="iwcQuote" style="border-left: 1px solid #00F; padding-left: 13px; margin-left: 0;" type="cite"><div class="mimetype-text-plain">Aditya Nandakumar via llvm-commits <llvm-commits@lists.llvm.org> writes:<br />> aditya_nandakumar created this revision.<br />> aditya_nandakumar added a reviewer: llvm-commits.<br />> aditya_nandakumar added a subscriber: llvm-commits.<br />> aditya_nandakumar set the repository for this revision to rL LLVM.<br />><br />> Canonicalizing negative constants out of expressions causes<br />> significant instruction count increase (not all of which is cleaned up<br />> by instcombine due to ordering of expressions)<br />><br />> The following patch tries factorize more combinations of FAdd/FSub<br />> instructions when unsafeAlgebra is set. This reduces the regression in<br />> codegen instructions from the above patch.<br />><br />> Some examples -<br />> Here op* is restricted to FAdd/FSub.<br />> // -> Transform (A op1 B) op2 C -> A op3 (B op4 C) if (B op4 C) factorizes<br />> //    Eg. (A + X*C1) + X * C2 -> A + X* (C1 + C2)<br />> // -> Transform A op1 (B op2 C) -> (A op3 B) op4 C) if (A op3 B) factorizes<br />> //    Eg. (A + X*C1) - X*C2 -> A + X *(C1-C2)<br />> // -> Transform ( A op1 B) op2 C -> (A op3 C) op4 B if (A op3 C) factorizes<br />> //    Eg. (X * C1 - B) + X * C2 -> X * (C1 - C2) - B<br />> // -> Transform A op1 (B op2 C) -> (A op3 C) op4 B<br />> //    Eg. X * C1 - (B + X * C2) -> X * (C1 - C2) - B<br />><br />> I'll add test cases post feedback.<br />><br />> Repository:<br />>   rL LLVM<br />><br />> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D12096&d=BQIBAg&c=eEvniauFctOgLOKGJOplqw&r=m0CyvKghw1pQ-GG-CTDpcR28dW-_mi50fdBDcbOm944&m=MMjCrcxf9rDguLTUCwquJbjug9dj4_6de4A9LDlf8Lw&s=RwwjvPxGfYShWlD8RvGwA1SPfYSUMU-y_hJAOtB8HsM&e=" target="l">https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D12096&d=BQIBAg&c=eEvniauFctOgLOKGJOplqw&r=m0CyvKghw1pQ-GG-CTDpcR28dW-_mi50fdBDcbOm944&m=MMjCrcxf9rDguLTUCwquJbjug9dj4_6de4A9LDlf8Lw&s=RwwjvPxGfYShWlD8RvGwA1SPfYSUMU-y_hJAOtB8HsM&e=</a> <br />><br />> Files:<br />>   lib/Transforms/InstCombine/InstCombineAddSub.cpp<br />><br />> Index: lib/Transforms/InstCombine/InstCombineAddSub.cpp<br />> ===================================================================<br />> --- lib/Transforms/InstCombine/InstCombineAddSub.cpp<br />> +++ lib/Transforms/InstCombine/InstCombineAddSub.cpp<br />> @@ -164,7 +164,8 @@<br />>  <br />>      Value *simplifyFAdd(AddendVect& V, unsigned InstrQuota);<br />>  <br />> -    Value *performFactorization(Instruction *I);<br />> +    Value *performFactorization(Value *Op0, Value *Op1, unsigned Opcode, FastMathFlags FMF);<br /><br />This line's long, and there are a few other places like it. Please run<br />clang-format on your changes.</div></blockquote><div>Ran clang-format in the latest diff. </div><blockquote cite="mid:m2pp2hk9v3.fsf@chronotis.apple.com" class="iwcQuote" style="border-left: 1px solid #00F; padding-left: 13px; margin-left: 0;" type="cite"><div class="mimetype-text-plain"><br /><br />> +    Value *performFactorizationAssociative(Instruction *I);<br />>  <br />>      /// Convert given addend to a Value<br />>      Value *createAddendVal(const FAddend &A, bool& NeedNeg);<br />> @@ -429,6 +430,184 @@<br />>    return BreakNum;<br />>  }<br />>  <br />> +// Here if we know that I is either FAdd or FSub<br />> +// see if we can factorize them individually.<br /><br />This comment doesn't make it clear whether this is a precondition or if<br />it's checked. Better to word this like "If I is either FAdd or FSub, see<br />if we can factorize ...".</div></blockquote><div>Makes sense - will update. </div><blockquote cite="mid:m2pp2hk9v3.fsf@chronotis.apple.com" class="iwcQuote" style="border-left: 1px solid #00F; padding-left: 13px; margin-left: 0;" type="cite"><div class="mimetype-text-plain"><br /><br />> +// The following should be possible as we would reach here if only<br />> +// unsafeAlgebra is enabled for this instruction.<br />> +// -> Transform (A op1 B) op2 C -> A op3 (B op4 C) if (B op4 C) factorizes<br />> +//    Eg. (A + X * C1) + X * C2 -> A + X * (C1 + C2)<br />> +// -> Transform A op1 (B op2 C) -> (A op3 B) op4 C) if (A op3 B) factorizes<br />> +//    Eg. (A + X * C1) - X * C2 -> A + X * (C1-C2)<br />> +// -> Transform ( A op1 B) op2 C -> (A op3 C) op4 B if (A op3 C) factorizes<br />> +//    Eg. (X * C1 - B) + X * C2 -> X * (C1 - C2) - B<br />> +// -> Transform A op1 (B op2 C) -> (A op3 C) op4 B<br />> +//    Eg. X * C1 - (B + X * C2) -> X * (C1 - C2) - B<br />> +Value *FAddCombine::performFactorizationAssociative(Instruction *I) {<br />> +  assert(I->hasUnsafeAlgebra() && "This method can't be called without unsafe algebra");<br />> +  if (I->getOpcode() != Instruction::FAdd && I->getOpcode() != Instruction::FSub)<br />> +    return nullptr;<br />> +  // Be overly conservative for now?<br /><br />This comment could use "TODO:", I think.</div></blockquote><div>Will do. </div><blockquote cite="mid:m2pp2hk9v3.fsf@chronotis.apple.com" class="iwcQuote" style="border-left: 1px solid #00F; padding-left: 13px; margin-left: 0;" type="cite"><div class="mimetype-text-plain"><br /><br />> +  if (I->getNumUses() != 1)<br />> +    return nullptr;<br />> +<br />> +  BinaryOperator *Op0 = dyn_cast<BinaryOperator>(I->getOperand(0));<br />> +  BinaryOperator *Op1 = dyn_cast<BinaryOperator>(I->getOperand(1));<br />> +  {<br />> +    // (A op B) op C -> A op (B op C) Simplify?<br />> +    bool Op0Factorizable = Op0 && (Op0->getOpcode() == Instruction::FAdd ||<br />> +                                   Op0->getOpcode() == Instruction::FSub);<br />> +    if (Op0Factorizable) {<br /><br />I'd probably drop a level of indentation and write this as:<br /><br />  if (bool Op0Factorizable = Op0 && (Op0->getOpcode() == Instruction::FAdd ||<br />                                     Op0->getOpcode() == Instruction::FSub)) {<br /><br />Similarly for the other three cases.<br /><br />Though, I don't see Op0Factorizable being used again, so you probably<br />don't even need the assignment.</div></blockquote><div>I just thought it is cleaner to read this way. I can remove it if required. </div><blockquote cite="mid:m2pp2hk9v3.fsf@chronotis.apple.com" class="iwcQuote" style="border-left: 1px solid #00F; padding-left: 13px; margin-left: 0;" type="cite"><div class="mimetype-text-plain"><br /><br />> +      unsigned FactorizeOpcode, FinalOpcode;<br />> +      Value *OpFactor0, *OpFactor1;<br />> +      bool IsOp0OpcodeAdd = (Op0->getOpcode() == Instruction::FAdd);<br />> +      bool IsIOpcodeAdd = (I->getOpcode() == Instruction::FAdd);<br />> +      Value *A = Op0->getOperand(0);<br />> +      Value *B = Op0->getOperand(1);<br />> +      Value *C = I->getOperand(1);<br />> +      // (a + b) + c -> a + ( b + c ) ?<br />> +      // (a + b) - c -> a + ( b - c ) ?<br />> +      if (IsOp0OpcodeAdd) {<br />> +        OpFactor0 = B;<br />> +        OpFactor1 = C;<br />> +        FactorizeOpcode = I->getOpcode();<br />> +        FinalOpcode = Instruction::FAdd;<br />> +      }<br />> +      // (a - b) + c -> a + (c - b) ?<br />> +      else if (!IsOp0OpcodeAdd && IsIOpcodeAdd) {<br />> +        OpFactor0 = C;<br />> +        OpFactor1 = B;<br />> +        FactorizeOpcode = Instruction::FSub;<br />> +        FinalOpcode = Instruction::FAdd;<br />> +      }<br />> +      // (a - b) - c -> a - (b + c) ?<br />> +      else {<br />> +        OpFactor0 = B;<br />> +        OpFactor1 = C;<br />> +        FactorizeOpcode = Instruction::FAdd;<br />> +        FinalOpcode = Instruction::FSub;<br />> +      }<br />> +      if (Value *V = performFactorization(OpFactor0, OpFactor1, FactorizeOpcode, I->getFastMathFlags())) {<br />> +        Value *NewV = (FinalOpcode == Instruction::FAdd) ? <br />> +                      createFAdd(A, V) :<br />> +                      createFSub(A, V);<br />> +        FastMathFlags Flags;<br />> +        Flags.setUnsafeAlgebra();<br />> +        Flags &= I->getFastMathFlags();<br />> +        Instruction *NewI = dyn_cast<Instruction>(NewV);<br />> +        assert(NewI && "We are expecting an instruction here ?? ");<br />> +        NewI->setFastMathFlags(Flags);<br />> +        return NewI;<br />> +      }<br />> +    }<br />> +  }<br />> +  {<br />> +    // Transform: "A op (B op C)" ==> "(A op B) op C" if "A op B" factorizes.<br />> +    bool Op1Factorizable = Op1 && (Op1->getOpcode() == Instruction::FAdd ||<br />> +                                   Op1->getOpcode() == Instruction::FSub);<br />> +    if (Op1Factorizable) {<br />> +      unsigned FactorizeOpcode, FinalOpcode;<br />> +      bool IsOp1OpcodeAdd = (Op1->getOpcode() == Instruction::FAdd);<br />> +      bool IsIOpcodeAdd = (I->getOpcode() == Instruction::FAdd);<br />> +      Value *A = I->getOperand(0);<br />> +      Value *B = Op1->getOperand(1);<br />> +      Value *C = Op1->getOperand(1);<br />> +      // A + (B + C) -> (A + B) + C factorizes?<br />> +      if (IsIOpcodeAdd && IsOp1OpcodeAdd) {<br />> +        FactorizeOpcode = FinalOpcode = Instruction::FAdd;<br />> +      }<br />> +      // A + (B - C) -> (A + B) - C factorizes?<br />> +      if (IsIOpcodeAdd && !IsOp1OpcodeAdd) {<br />> +        FactorizeOpcode = Instruction::FAdd;<br />> +        FinalOpcode = Instruction::FSub;<br />> +      }<br />> +      // A - (B + C) -> (A - B) - C factorizes?<br />> +      if (!IsIOpcodeAdd && IsOp1OpcodeAdd) {<br />> +        FactorizeOpcode = FinalOpcode = Instruction::FSub;<br />> +      }<br />> +      // A - (B - C) -> (A - B) + C factorizes?<br />> +      if (!IsIOpcodeAdd && !IsOp1OpcodeAdd) {<br />> +        FactorizeOpcode = Instruction::FSub;<br />> +        FinalOpcode = Instruction::FAdd;<br />> +      }<br />> +      if (Value *V = performFactorization(A, B, FactorizeOpcode, I->getFastMathFlags())) {<br />> +        Value *NewV = (FinalOpcode == Instruction::FAdd) ? <br />> +                        createFAdd(V, C)<br />> +                        : createFSub(V, C);<br />> +        FastMathFlags Flags;<br />> +        Flags.setUnsafeAlgebra();<br />> +        Flags &= I->getFastMathFlags();<br />> +        Instruction *NewI = dyn_cast<Instruction>(NewV);<br />> +        assert(NewI && "We are expecting an instruction here ?? ");<br />> +        NewI->setFastMathFlags(Flags);<br />> +        return NewI;<br />> +      }<br />> +    }<br />> +  }<br />> +  {<br />> +    // We know that op1 and op2 can only be FAdd or FSub<br />> +    // (A op1 B) op2 C -> (A op2 C) op1 B factorizes? <br />> +    bool Op0Valid = Op0 && (Op0->getOpcode() == Instruction::FAdd ||<br />> +                            Op0->getOpcode() == Instruction::FSub);<br />> +    if (Op0Valid) {<br />> +      Value *A = Op0->getOperand(0);<br />> +      Value *B = Op0->getOperand(1);<br />> +      Value *C = I->getOperand(1);<br />> +      if (Value *V = performFactorization(A, C, I->getOpcode(), I->getFastMathFlags())) {<br />> +        Value *NewV = (Op0->getOpcode() == Instruction::FAdd) ? <br />> +                        createFAdd(V, B)<br />> +                        : createFSub(V, B);<br />> +        FastMathFlags Flags;<br />> +        Flags.setUnsafeAlgebra();<br />> +        Flags &= I->getFastMathFlags();<br />> +        Instruction *NewI = dyn_cast<Instruction>(NewV);<br />> +        assert(NewI && "We are expecting an instruction here ?? ");<br />> +        NewI->setFastMathFlags(Flags);<br />> +        return NewI;<br />> +      }<br />> +    } <br />> +  }<br />> +  {<br />> +    // A op1 (B op2 C) -> (A op2 C) op1 B factorizes?<br />> +    bool Op1Valid = Op1 && (Op1->getOpcode() == Instruction::FAdd ||<br />> +                            Op1->getOpcode() == Instruction::FSub);<br />> +    if (Op1Valid) {<br />> +      Value *A = I->getOperand(0);<br />> +      Value *B = Op1->getOperand(0);<br />> +      Value *C = Op1->getOperand(1);<br />> +      unsigned FactorizeOpcode, FinalOpcode;<br />> +      bool IsOp1OpcodeAdd = (Op1->getOpcode() == Instruction::FAdd);<br />> +      bool IsIOpcodeAdd = (I->getOpcode() == Instruction::FAdd);<br />> +      // A + (B + C) -> (A + C) + B simplifies?<br />> +      // A + (B - C) -> (A - C) + B simplifies?<br />> +      if (IsIOpcodeAdd) {<br />> +        FactorizeOpcode = Op1->getOpcode();<br />> +        FinalOpcode = Instruction::FAdd;<br />> +      }<br />> +      // A - (B + C) -> (A - C) - B<br />> +      else if(IsOp1OpcodeAdd) {<br />> +        FactorizeOpcode = FinalOpcode = Instruction::FSub;<br />> +      } else {<br />> +        // A - (B - C) -> (A + C) - B<br />> +        FactorizeOpcode = Instruction::FAdd;<br />> +        FinalOpcode = Instruction::FSub;<br />> +      }<br />> +      if (Value * V = performFactorization(A, C, FactorizeOpcode, I->getFastMathFlags())) {<br />> +        Value *NewV = (FinalOpcode == Instruction::FAdd) ?<br />> +                      createFAdd(V, B) :<br />> +                      createFSub(V, B);<br />> +        FastMathFlags Flags;<br />> +        Flags.setUnsafeAlgebra();<br />> +        Flags &= I->getFastMathFlags();<br />> +        Instruction *NewI = dyn_cast<Instruction>(NewV);<br />> +        assert(NewI && "We are expecting an instruction here ?? ");<br />> +        NewI->setFastMathFlags(Flags);<br />> +        return NewI;<br />> +      }<br />> +    }<br />> +  }<br />> +  return nullptr;<br />> +}<br />> +<br />>  // Try to perform following optimization on the input instruction I. Return the<br />>  // simplified expression if was successful; otherwise, return 0.<br />>  //<br />> @@ -437,12 +616,13 @@<br />>  //   (x * y) +/- (x * z)               x * (y +/- z)<br />>  //   (y / x) +/- (z / x)               (y +/- z) / x<br />>  //<br />> -Value *FAddCombine::performFactorization(Instruction *I) {<br />> -  assert((I->getOpcode() == Instruction::FAdd ||<br />> -          I->getOpcode() == Instruction::FSub) && "Expect add/sub");<br />> +Value *FAddCombine::performFactorization(Value *Op0, Value *Op1, unsigned Opcode,<br />> +                                         FastMathFlags FMF) {<br />> +  assert((Opcode == Instruction::FAdd ||<br />> +          Opcode == Instruction::FSub) && "Expect add/sub");<br /><br />The change to pass in the parts of I instead of I itself seems unrelated<br />or non-functional. It also seems odd that the signatures of<br />performFactorization and performFactorizationAssociative differ. I'd<br />probably just drop this part of the change.</div></blockquote><div>This change in signature for performFactorization is required.</div><div>Previously it accepted Instruction and would extract the two operands to see if it could be factorized but now we try to factorize values across different instructions.</div><div>Also the opcode and the operand order for factorization can change. For eg</div><div>(a - b) + c -> a + (c - b) which would roughly translate to factorize(c , b, FSub).</div><blockquote cite="mid:m2pp2hk9v3.fsf@chronotis.apple.com" class="iwcQuote" style="border-left: 1px solid #00F; padding-left: 13px; margin-left: 0;" type="cite"><div class="mimetype-text-plain"><br /><br />>  <br />> -  Instruction *I0 = dyn_cast<Instruction>(I->getOperand(0));<br />> -  Instruction *I1 = dyn_cast<Instruction>(I->getOperand(1));<br />> +  Instruction *I0 = dyn_cast<Instruction>(Op0);<br />> +  Instruction *I1 = dyn_cast<Instruction>(Op1);<br />>  <br />>    if (!I0 || !I1 || I0->getOpcode() != I1->getOpcode())<br />>      return nullptr;<br />> @@ -487,11 +667,11 @@<br />>  <br />>    FastMathFlags Flags;<br />>    Flags.setUnsafeAlgebra();<br />> -  if (I0) Flags &= I->getFastMathFlags();<br />> -  if (I1) Flags &= I->getFastMathFlags();<br />> +  if (I0) Flags &= FMF;<br />> +  if (I1) Flags &= FMF;<br />>  <br />>    // Create expression "NewAddSub = AddSub0 +/- AddsSub1"<br />> -  Value *NewAddSub = (I->getOpcode() == Instruction::FAdd) ?<br />> +  Value *NewAddSub = (Opcode == Instruction::FAdd) ?<br />>                        createFAdd(AddSub0, AddSub1) :<br />>                        createFSub(AddSub0, AddSub1);<br />>    if (ConstantFP *CFP = dyn_cast<ConstantFP>(NewAddSub)) {<br />> @@ -598,7 +778,12 @@<br />>    }<br />>  <br />>    // step 6: Try factorization as the last resort,<br />> -  return performFactorization(I);<br />> +  if (Value *V = performFactorization(I->getOperand(0),<br />> +                                      I->getOperand(1),<br />> +                                      I->getOpcode(),<br />> +                                      I->getFastMathFlags()))<br />> +    return V;<br />> +  return performFactorizationAssociative(I);<br />>  }<br />>  <br />>  Value *FAddCombine::simplifyFAdd(AddendVect& Addends, unsigned InstrQuota) {<br />> _______________________________________________<br />> llvm-commits mailing list<br />> llvm-commits@lists.llvm.org<br />> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Dcommits&d=BQIBAg&c=eEvniauFctOgLOKGJOplqw&r=m0CyvKghw1pQ-GG-CTDpcR28dW-_mi50fdBDcbOm944&m=MMjCrcxf9rDguLTUCwquJbjug9dj4_6de4A9LDlf8Lw&s=cZfHdwr-BIwvE2STj4dT-rVWoH6V03QVxT6J_s5k9Qo&e=" target="l">https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Dcommits&d=BQIBAg&c=eEvniauFctOgLOKGJOplqw&r=m0CyvKghw1pQ-GG-CTDpcR28dW-_mi50fdBDcbOm944&m=MMjCrcxf9rDguLTUCwquJbjug9dj4_6de4A9LDlf8Lw&s=cZfHdwr-BIwvE2STj4dT-rVWoH6V03QVxT6J_s5k9Qo&e=</a> <br /></div></blockquote>