[PATCH] D12096: [InstCombineAddSub opportunities]: More opportunities to factorize FAdd/FSub when unsafeAlgebra is present for Inst

[Aditya Nandakumar via llvm-commits]

On 08/20/15 01:01 PM, Justin Bogner  <mail at justinbogner.com> wrote: 
> 
> Aditya Nandakumar via llvm-commits <llvm-commits at lists.llvm.org> writes:
> > aditya_nandakumar created this revision.
> > aditya_nandakumar added a reviewer: llvm-commits.
> > aditya_nandakumar added a subscriber: llvm-commits.
> > aditya_nandakumar set the repository for this revision to rL LLVM.
> >
> > Canonicalizing negative constants out of expressions causes
> > significant instruction count increase (not all of which is cleaned up
> > by instcombine due to ordering of expressions)
> >
> > The following patch tries factorize more combinations of FAdd/FSub
> > instructions when unsafeAlgebra is set. This reduces the regression in
> > codegen instructions from the above patch.
> >
> > Some examples -
> > Here op* is restricted to FAdd/FSub.
> > // -> Transform (A op1 B) op2 C -> A op3 (B op4 C) if (B op4 C) factorizes
> > // Eg. (A + X*C1) + X * C2 -> A + X* (C1 + C2)
> > // -> Transform A op1 (B op2 C) -> (A op3 B) op4 C) if (A op3 B) factorizes
> > // Eg. (A + X*C1) - X*C2 -> A + X *(C1-C2)
> > // -> Transform ( A op1 B) op2 C -> (A op3 C) op4 B if (A op3 C) factorizes
> > // Eg. (X * C1 - B) + X * C2 -> X * (C1 - C2) - B
> > // -> Transform A op1 (B op2 C) -> (A op3 C) op4 B
> > // Eg. X * C1 - (B + X * C2) -> X * (C1 - C2) - B
> >
> > I'll add test cases post feedback.
> >
> > Repository:
> > rL LLVM
> >
> > 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= 
> >
> > Files:
> > lib/Transforms/InstCombine/InstCombineAddSub.cpp
> >
> > Index: lib/Transforms/InstCombine/InstCombineAddSub.cpp
> > ===================================================================
> > --- lib/Transforms/InstCombine/InstCombineAddSub.cpp
> > +++ lib/Transforms/InstCombine/InstCombineAddSub.cpp
> > @@ -164,7 +164,8 @@
> > 
> > Value *simplifyFAdd(AddendVect& V, unsigned InstrQuota);
> > 
> > - Value *performFactorization(Instruction *I);
> > + Value *performFactorization(Value *Op0, Value *Op1, unsigned Opcode, FastMathFlags FMF);
> 
> This line's long, and there are a few other places like it. Please run
> clang-format on your changes.
> 

Ran clang-format in the latest diff. 

> 
> 
> 
> > + Value *performFactorizationAssociative(Instruction *I);
> > 
> > /// Convert given addend to a Value
> > Value *createAddendVal(const FAddend &A, bool& NeedNeg);
> > @@ -429,6 +430,184 @@
> > return BreakNum;
> > }
> > 
> > +// Here if we know that I is either FAdd or FSub
> > +// see if we can factorize them individually.
> 
> This comment doesn't make it clear whether this is a precondition or if
> it's checked. Better to word this like "If I is either FAdd or FSub, see
> if we can factorize ...".
> 

Makes sense - will update. 

> 
> 
> 
> > +// The following should be possible as we would reach here if only
> > +// unsafeAlgebra is enabled for this instruction.
> > +// -> Transform (A op1 B) op2 C -> A op3 (B op4 C) if (B op4 C) factorizes
> > +// Eg. (A + X * C1) + X * C2 -> A + X * (C1 + C2)
> > +// -> Transform A op1 (B op2 C) -> (A op3 B) op4 C) if (A op3 B) factorizes
> > +// Eg. (A + X * C1) - X * C2 -> A + X * (C1-C2)
> > +// -> Transform ( A op1 B) op2 C -> (A op3 C) op4 B if (A op3 C) factorizes
> > +// Eg. (X * C1 - B) + X * C2 -> X * (C1 - C2) - B
> > +// -> Transform A op1 (B op2 C) -> (A op3 C) op4 B
> > +// Eg. X * C1 - (B + X * C2) -> X * (C1 - C2) - B
> > +Value *FAddCombine::performFactorizationAssociative(Instruction *I) {
> > + assert(I->hasUnsafeAlgebra() && "This method can't be called without unsafe algebra");
> > + if (I->getOpcode() != Instruction::FAdd && I->getOpcode() != Instruction::FSub)
> > + return nullptr;
> > + // Be overly conservative for now?
> 
> This comment could use "TODO:", I think.
> 

Will do. 

> 
> 
> 
> > + if (I->getNumUses() != 1)
> > + return nullptr;
> > +
> > + BinaryOperator *Op0 = dyn_cast<BinaryOperator>(I->getOperand(0));
> > + BinaryOperator *Op1 = dyn_cast<BinaryOperator>(I->getOperand(1));
> > + {
> > + // (A op B) op C -> A op (B op C) Simplify?
> > + bool Op0Factorizable = Op0 && (Op0->getOpcode() == Instruction::FAdd ||
> > + Op0->getOpcode() == Instruction::FSub);
> > + if (Op0Factorizable) {
> 
> I'd probably drop a level of indentation and write this as:
> 
>  if (bool Op0Factorizable = Op0 && (Op0->getOpcode() == Instruction::FAdd ||
>  Op0->getOpcode() == Instruction::FSub)) {
> 
> Similarly for the other three cases.
> 
> Though, I don't see Op0Factorizable being used again, so you probably
> don't even need the assignment.
> 

I just thought it is cleaner to read this way. I can remove it if required. 

> 
> 
> 
> > + unsigned FactorizeOpcode, FinalOpcode;
> > + Value *OpFactor0, *OpFactor1;
> > + bool IsOp0OpcodeAdd = (Op0->getOpcode() == Instruction::FAdd);
> > + bool IsIOpcodeAdd = (I->getOpcode() == Instruction::FAdd);
> > + Value *A = Op0->getOperand(0);
> > + Value *B = Op0->getOperand(1);
> > + Value *C = I->getOperand(1);
> > + // (a + b) + c -> a + ( b + c ) ?
> > + // (a + b) - c -> a + ( b - c ) ?
> > + if (IsOp0OpcodeAdd) {
> > + OpFactor0 = B;
> > + OpFactor1 = C;
> > + FactorizeOpcode = I->getOpcode();
> > + FinalOpcode = Instruction::FAdd;
> > + }
> > + // (a - b) + c -> a + (c - b) ?
> > + else if (!IsOp0OpcodeAdd && IsIOpcodeAdd) {
> > + OpFactor0 = C;
> > + OpFactor1 = B;
> > + FactorizeOpcode = Instruction::FSub;
> > + FinalOpcode = Instruction::FAdd;
> > + }
> > + // (a - b) - c -> a - (b + c) ?
> > + else {
> > + OpFactor0 = B;
> > + OpFactor1 = C;
> > + FactorizeOpcode = Instruction::FAdd;
> > + FinalOpcode = Instruction::FSub;
> > + }
> > + if (Value *V = performFactorization(OpFactor0, OpFactor1, FactorizeOpcode, I->getFastMathFlags())) {
> > + Value *NewV = (FinalOpcode == Instruction::FAdd) ? 
> > + createFAdd(A, V) :
> > + createFSub(A, V);
> > + FastMathFlags Flags;
> > + Flags.setUnsafeAlgebra();
> > + Flags &= I->getFastMathFlags();
> > + Instruction *NewI = dyn_cast<Instruction>(NewV);
> > + assert(NewI && "We are expecting an instruction here ?? ");
> > + NewI->setFastMathFlags(Flags);
> > + return NewI;
> > + }
> > + }
> > + }
> > + {
> > + // Transform: "A op (B op C)" ==> "(A op B) op C" if "A op B" factorizes.
> > + bool Op1Factorizable = Op1 && (Op1->getOpcode() == Instruction::FAdd ||
> > + Op1->getOpcode() == Instruction::FSub);
> > + if (Op1Factorizable) {
> > + unsigned FactorizeOpcode, FinalOpcode;
> > + bool IsOp1OpcodeAdd = (Op1->getOpcode() == Instruction::FAdd);
> > + bool IsIOpcodeAdd = (I->getOpcode() == Instruction::FAdd);
> > + Value *A = I->getOperand(0);
> > + Value *B = Op1->getOperand(1);
> > + Value *C = Op1->getOperand(1);
> > + // A + (B + C) -> (A + B) + C factorizes?
> > + if (IsIOpcodeAdd && IsOp1OpcodeAdd) {
> > + FactorizeOpcode = FinalOpcode = Instruction::FAdd;
> > + }
> > + // A + (B - C) -> (A + B) - C factorizes?
> > + if (IsIOpcodeAdd && !IsOp1OpcodeAdd) {
> > + FactorizeOpcode = Instruction::FAdd;
> > + FinalOpcode = Instruction::FSub;
> > + }
> > + // A - (B + C) -> (A - B) - C factorizes?
> > + if (!IsIOpcodeAdd && IsOp1OpcodeAdd) {
> > + FactorizeOpcode = FinalOpcode = Instruction::FSub;
> > + }
> > + // A - (B - C) -> (A - B) + C factorizes?
> > + if (!IsIOpcodeAdd && !IsOp1OpcodeAdd) {
> > + FactorizeOpcode = Instruction::FSub;
> > + FinalOpcode = Instruction::FAdd;
> > + }
> > + if (Value *V = performFactorization(A, B, FactorizeOpcode, I->getFastMathFlags())) {
> > + Value *NewV = (FinalOpcode == Instruction::FAdd) ? 
> > + createFAdd(V, C)
> > + : createFSub(V, C);
> > + FastMathFlags Flags;
> > + Flags.setUnsafeAlgebra();
> > + Flags &= I->getFastMathFlags();
> > + Instruction *NewI = dyn_cast<Instruction>(NewV);
> > + assert(NewI && "We are expecting an instruction here ?? ");
> > + NewI->setFastMathFlags(Flags);
> > + return NewI;
> > + }
> > + }
> > + }
> > + {
> > + // We know that op1 and op2 can only be FAdd or FSub
> > + // (A op1 B) op2 C -> (A op2 C) op1 B factorizes? 
> > + bool Op0Valid = Op0 && (Op0->getOpcode() == Instruction::FAdd ||
> > + Op0->getOpcode() == Instruction::FSub);
> > + if (Op0Valid) {
> > + Value *A = Op0->getOperand(0);
> > + Value *B = Op0->getOperand(1);
> > + Value *C = I->getOperand(1);
> > + if (Value *V = performFactorization(A, C, I->getOpcode(), I->getFastMathFlags())) {
> > + Value *NewV = (Op0->getOpcode() == Instruction::FAdd) ? 
> > + createFAdd(V, B)
> > + : createFSub(V, B);
> > + FastMathFlags Flags;
> > + Flags.setUnsafeAlgebra();
> > + Flags &= I->getFastMathFlags();
> > + Instruction *NewI = dyn_cast<Instruction>(NewV);
> > + assert(NewI && "We are expecting an instruction here ?? ");
> > + NewI->setFastMathFlags(Flags);
> > + return NewI;
> > + }
> > + } 
> > + }
> > + {
> > + // A op1 (B op2 C) -> (A op2 C) op1 B factorizes?
> > + bool Op1Valid = Op1 && (Op1->getOpcode() == Instruction::FAdd ||
> > + Op1->getOpcode() == Instruction::FSub);
> > + if (Op1Valid) {
> > + Value *A = I->getOperand(0);
> > + Value *B = Op1->getOperand(0);
> > + Value *C = Op1->getOperand(1);
> > + unsigned FactorizeOpcode, FinalOpcode;
> > + bool IsOp1OpcodeAdd = (Op1->getOpcode() == Instruction::FAdd);
> > + bool IsIOpcodeAdd = (I->getOpcode() == Instruction::FAdd);
> > + // A + (B + C) -> (A + C) + B simplifies?
> > + // A + (B - C) -> (A - C) + B simplifies?
> > + if (IsIOpcodeAdd) {
> > + FactorizeOpcode = Op1->getOpcode();
> > + FinalOpcode = Instruction::FAdd;
> > + }
> > + // A - (B + C) -> (A - C) - B
> > + else if(IsOp1OpcodeAdd) {
> > + FactorizeOpcode = FinalOpcode = Instruction::FSub;
> > + } else {
> > + // A - (B - C) -> (A + C) - B
> > + FactorizeOpcode = Instruction::FAdd;
> > + FinalOpcode = Instruction::FSub;
> > + }
> > + if (Value * V = performFactorization(A, C, FactorizeOpcode, I->getFastMathFlags())) {
> > + Value *NewV = (FinalOpcode == Instruction::FAdd) ?
> > + createFAdd(V, B) :
> > + createFSub(V, B);
> > + FastMathFlags Flags;
> > + Flags.setUnsafeAlgebra();
> > + Flags &= I->getFastMathFlags();
> > + Instruction *NewI = dyn_cast<Instruction>(NewV);
> > + assert(NewI && "We are expecting an instruction here ?? ");
> > + NewI->setFastMathFlags(Flags);
> > + return NewI;
> > + }
> > + }
> > + }
> > + return nullptr;
> > +}
> > +
> > // Try to perform following optimization on the input instruction I. Return the
> > // simplified expression if was successful; otherwise, return 0.
> > //
> > @@ -437,12 +616,13 @@
> > // (x * y) +/- (x * z) x * (y +/- z)
> > // (y / x) +/- (z / x) (y +/- z) / x
> > //
> > -Value *FAddCombine::performFactorization(Instruction *I) {
> > - assert((I->getOpcode() == Instruction::FAdd ||
> > - I->getOpcode() == Instruction::FSub) && "Expect add/sub");
> > +Value *FAddCombine::performFactorization(Value *Op0, Value *Op1, unsigned Opcode,
> > + FastMathFlags FMF) {
> > + assert((Opcode == Instruction::FAdd ||
> > + Opcode == Instruction::FSub) && "Expect add/sub");
> 
> The change to pass in the parts of I instead of I itself seems unrelated
> or non-functional. It also seems odd that the signatures of
> performFactorization and performFactorizationAssociative differ. I'd
> probably just drop this part of the change.
> 

This change in signature for performFactorization is required.
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.
Also the opcode and the operand order for factorization can change. For eg
(a - b) + c -> a + (c - b) which would roughly translate to factorize(c , b, FSub).

> 
> 
> 
> > 
> > - Instruction *I0 = dyn_cast<Instruction>(I->getOperand(0));
> > - Instruction *I1 = dyn_cast<Instruction>(I->getOperand(1));
> > + Instruction *I0 = dyn_cast<Instruction>(Op0);
> > + Instruction *I1 = dyn_cast<Instruction>(Op1);
> > 
> > if (!I0 || !I1 || I0->getOpcode() != I1->getOpcode())
> > return nullptr;
> > @@ -487,11 +667,11 @@
> > 
> > FastMathFlags Flags;
> > Flags.setUnsafeAlgebra();
> > - if (I0) Flags &= I->getFastMathFlags();
> > - if (I1) Flags &= I->getFastMathFlags();
> > + if (I0) Flags &= FMF;
> > + if (I1) Flags &= FMF;
> > 
> > // Create expression "NewAddSub = AddSub0 +/- AddsSub1"
> > - Value *NewAddSub = (I->getOpcode() == Instruction::FAdd) ?
> > + Value *NewAddSub = (Opcode == Instruction::FAdd) ?
> > createFAdd(AddSub0, AddSub1) :
> > createFSub(AddSub0, AddSub1);
> > if (ConstantFP *CFP = dyn_cast<ConstantFP>(NewAddSub)) {
> > @@ -598,7 +778,12 @@
> > }
> > 
> > // step 6: Try factorization as the last resort,
> > - return performFactorization(I);
> > + if (Value *V = performFactorization(I->getOperand(0),
> > + I->getOperand(1),
> > + I->getOpcode(),
> > + I->getFastMathFlags()))
> > + return V;
> > + return performFactorizationAssociative(I);
> > }
> > 
> > Value *FAddCombine::simplifyFAdd(AddendVect& Addends, unsigned InstrQuota) {
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > 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= 
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150820/9fdff1bc/attachment.html>