[PATCH] Reassociate x + -0.1234 * y into x - 0.1234 * y

[Duncan P. N. Exon Smith]

On 2014 Apr 12, at 07:43, Erik Verbruggen <erik.verbruggen at me.com> wrote:

> [PATCH] Reassociate x + -0.1234 * y into x - 0.1234 * y
> 
> This resolves a README entry.
> 
> <0001-Reassociate-x-0.1234-y-into-x-0.1234-y.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

Hi Erik,

Thanks for working on this!

I have a couple of questions and a bunch of style nits.  The style
issues mostly fall into the "variables should be capitalized" and
"clang-format" [1] categories.

[1]: http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

> diff --git a/lib/Transforms/Scalar/Reassociate.cpp b/lib/Transforms/Scalar/Reassociate.cpp
> index b6b4d97..8220ff3 100644
> --- a/lib/Transforms/Scalar/Reassociate.cpp
> +++ b/lib/Transforms/Scalar/Reassociate.cpp
> @@ -192,6 +192,7 @@ namespace {
>      Value *OptimizeMul(BinaryOperator *I, SmallVectorImpl<ValueEntry> &Ops);
>      Value *RemoveFactorFromExpression(Value *V, Value *Factor);
>      void EraseInst(Instruction *I);
> +    void optimizeFAddNegExpr(ConstantFP *ConstOperand, Instruction *I, int OperandNr);

80 column?

>      void OptimizeInst(Instruction *I);
>    };
>  }
> @@ -1787,6 +1788,31 @@ void Reassociate::EraseInst(Instruction *I) {
>      }
>  }
>  
> +void Reassociate::optimizeFAddNegExpr(ConstantFP *ConstOperand, Instruction *I,
> +                                      int OperandNr)
> +{

This brace should be at the end of the previous line.

> +  // Change the sign of the constant.
> +  APFloat val = ConstOperand->getValueAPF();

Style is to use Val, not val.

> +  val.changeSign();
> +  I->setOperand(0, ConstantFP::get(ConstOperand->getContext(), val));
> +
> +  Instruction *parent = I->user_back();

Also Parent instead of parent.

I think some assertions here might help readability.  E.g., that "I"
only has one user.

> +  Value *OtherOperand = parent->getOperand(1 - OperandNr);
> +  if (Instruction *OtherInstruction = dyn_cast<Instruction>(OtherOperand))
> +    RedoInsts.insert(OtherInstruction);

This is probably right, but it's not obvious to me.  Why should you redo
the other operand?

> +  BinaryOperator *NI;
> +  if (parent->getOpcode() == Instruction::FAdd) {
> +    NI = BinaryOperator::CreateFSub(OtherOperand, I);
> +  } else {
> +    NI = BinaryOperator::CreateFAdd(OtherOperand, I);
> +  }

No braces here.

> +  NI->insertBefore(parent);
> +  NI->setName(parent->getName() + ".repl");
> +  parent->replaceAllUsesWith(NI);
> +  NI->setDebugLoc(I->getDebugLoc());
> +  MadeChange = true;
> +}
> +
>  /// OptimizeInst - Inspect and optimize the given instruction. Note that erasing
>  /// instructions is not allowed.
>  void Reassociate::OptimizeInst(Instruction *I) {
> @@ -1829,6 +1855,22 @@ void Reassociate::OptimizeInst(Instruction *I) {
>        I->setOperand(1, LHS);
>      }
>  
> +    if (I->getOpcode() == Instruction::FMul || I->getOpcode() == Instruction::FDiv) {

80 column?

It looks to me like the FDiv check is dead code.  Should this new code
be earlier in the containing block?

> +      if (ConstantFP *LHSConst = dyn_cast<ConstantFP>(I->getOperand(0))) {
> +        Instruction *parent = I->user_back();

Again, Parent not parent.

Also, this should be inside the succeeding if.

> +        if (LHSConst->isNegative() && I->hasOneUse()) {
> +          if (parent->getOpcode() == Instruction::FAdd) {
> +            if (parent->getOperand(0) == I )

Remove the space before the closing parenthesis.

> +              optimizeFAddNegExpr(LHSConst, I, 0);
> +            else if (parent->getOperand(1) == I)
> +              optimizeFAddNegExpr(LHSConst, I, 1);
> +          } else if (parent->getOpcode() == Instruction::FSub)
> +            if (parent->getOperand(1) == I)
> +              optimizeFAddNegExpr(LHSConst, I, 1);
> +        }
> +      }
> +    }
> +
>      return;
>    }
>  
> diff --git a/test/Transforms/Reassociate/liftsign.ll b/test/Transforms/Reassociate/liftsign.ll
> new file mode 100644
> index 0000000..9437094
> --- /dev/null
> +++ b/test/Transforms/Reassociate/liftsign.ll
> @@ -0,0 +1,35 @@
> +; RUN: opt < %s -reassociate -gvn -S < %s | FileCheck %s
> +
> +; (x + 0.1234 * y) * (x + -0.1234 * y) -> (x + 0.1234 * y) * (x - 0.1234 * y)
> +; so CSE can simplify it further

Test coverage for FDiv?

> +define double @lift_sign1(double %x, double %y) nounwind readnone ssp uwtable {
> +; CHECK-LABEL: @lift_sign1(
> +  %mul = fmul double %y, 1.234000e-01
> +  %add = fadd double %mul, %x
> +  %mul1 = fmul double %y, -1.234000e-01
> +  %add2 = fadd double %mul1, %x
> +  %mul3 = fmul double %add, %add2
> +  ret double %mul3
> +}
> +
> +; (x + -0.1234 * y) * (x + -0.1234 * y) -> (x - 0.1234 * y) * (x - 0.1234 * y)
> +define double @lift_sign2(double %x, double %y) nounwind readnone ssp uwtable {
> +; CHECK-LABEL: @lift_sign2(
> +  %mul = fmul double %y, -1.234000e-01
> +  %add = fadd double %mul, %x
> +  %mul1 = fmul double %y, -1.234000e-01
> +  %add2 = fadd double %mul1, %x
> +  %mul3 = fmul double %add, %add2
> +  ret double %mul3
> +}
> +
> +; (x + 0.1234 * y) * (x - -0.1234 * y) -> (x + 0.1234 * y) * (x + 0.1234 * y)
> +define double @lift_sign3(double %x, double %y) nounwind readnone ssp uwtable {
> +; CHECK-LABEL: @lift_sign3(
> +  %mul = fmul double %y, 1.234000e-01
> +  %add = fadd double %mul, %x
> +  %mul1 = fmul double %y, -1.234000e-01
> +  %add2 = fsub double %x, %mul1
> +  %mul3 = fmul double %add, %add2
> +  ret double %mul3
> +}
> -- 
> 1.8.5.4
> 
>