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

[Duncan P. N. Exon Smith]

On 2014-Apr-21, at 5:21, Erik Verbruggen <erik.verbruggen at me.com> wrote:

> Hi Duncan,
> 
> Thanks for the feedback! Fixed everything you brought up, and I attached a new patch.

This is getting close.  I have a few more comments.

> A couple of comments inline:
> 
> On 12 Apr 2014, at 22:54, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
>> 
>> On 2014 Apr 12, at 07:43, Erik Verbruggen <erik.verbruggen at me.com> wrote:
>> 
>>> +  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?
> 
> Now that the parent has changed, that other operand might be eligible for another optimisation. That said, I checked the available optimisations for fadd/fsub and I can't find any that might apply. So, I removed it in the new patch.

Thanks for the explanation.  I'm happy to go either way with this.

Some comments below on your newest version.

> diff --git a/lib/Transforms/Scalar/Reassociate.cpp b/lib/Transforms/Scalar/Reassociate.cpp
> index b6b4d97..934b2e1 100644
> --- a/lib/Transforms/Scalar/Reassociate.cpp
> +++ b/lib/Transforms/Scalar/Reassociate.cpp
> @@ -192,6 +192,8 @@ 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);
>      void OptimizeInst(Instruction *I);
>    };
>  }
> @@ -1787,6 +1789,32 @@ void Reassociate::EraseInst(Instruction *I) {
>      }
>  }
>  
> +void Reassociate::optimizeFAddNegExpr(ConstantFP *ConstOperand, Instruction *I,
> +                                      int OperandNr) {
> +  // Change the sign of the constant.
> +  APFloat Val = ConstOperand->getValueAPF();
> +  Val.changeSign();
> +  I->setOperand(0, ConstantFP::get(ConstOperand->getContext(), Val));
> +
> +  assert(I->hasOneUse() && "Only a single use can be replaced.");
> +  Instruction *Parent = I->user_back();
> +
> +  assert(
> +      Parent->getOpcode() == Instruction::FAdd ||
> +      (Parent->getOpcode() == Instruction::FSub && Parent->getOperand(1) == I));
> +  Value *OtherOperand = Parent->getOperand(1 - OperandNr);
> +  BinaryOperator *NI;
> +  if (Parent->getOpcode() == Instruction::FAdd)
> +    NI = BinaryOperator::CreateFSub(OtherOperand, I);
> +  else
> +    NI = BinaryOperator::CreateFAdd(OtherOperand, I);

I find that assert hard to read.  I wonder if there's a way to
reorganize the code to make it clearer?  E.g., what do you think of
this?

    Value *OtherOperand = Parent->getOperand(1 - OperandNr);
    BinaryOperator *NI;
    if (Parent->getOpcode() == Instruction::FAdd)
      NI = BinaryOperator::CreateFSub(OtherOperand, I);
    else {
      assert(Parent->getOpcode() == Instruction::FSub);
      assert(OperandNr == 1);
      NI = BinaryOperator::CreateFAdd(OtherOperand, I);
    }

It would be great if you came up with useful assert(... && "messages")
too.

> +  NI->insertBefore(Parent);
> +  NI->setName(Parent->getName() + ".repl");
> +  Parent->replaceAllUsesWith(NI);

I think you're missing:

    if (Parent->getParent())
      Parent->eraseFromParent();

> +  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) {
> @@ -1808,25 +1836,45 @@ void Reassociate::OptimizeInst(Instruction *I) {
>        I = NI;
>      }
>  
> -  // Floating point binary operators are not associative, but we can still
> -  // commute (some) of them, to canonicalize the order of their operands.
> -  // This can potentially expose more CSE opportunities, and makes writing
> -  // other transformations simpler.
>    if ((I->getType()->isFloatingPointTy() || I->getType()->isVectorTy())) {
> -    // FAdd and FMul can be commuted.
> -    if (I->getOpcode() != Instruction::FMul &&
> -        I->getOpcode() != Instruction::FAdd)
> -      return;
> +    // Floating point binary operators are not associative, but we can still
> +    // commute (some) of them, to canonicalize the order of their operands.
> +    // This can potentially expose more CSE opportunities, and makes writing
> +    // other transformations simpler.
>  
> -    Value *LHS = I->getOperand(0);
> -    Value *RHS = I->getOperand(1);
> -    unsigned LHSRank = getRank(LHS);
> -    unsigned RHSRank = getRank(RHS);
> +    // FAdd and FMul can be commuted.
> +    if (I->getOpcode() == Instruction::FMul ||
> +        I->getOpcode() == Instruction::FAdd) {
> +      Value *LHS = I->getOperand(0);
> +      Value *RHS = I->getOperand(1);
> +      unsigned LHSRank = getRank(LHS);
> +      unsigned RHSRank = getRank(RHS);
> +
> +      // Sort the operands by rank.
> +      if (RHSRank < LHSRank) {
> +        I->setOperand(0, RHS);
> +        I->setOperand(1, LHS);
> +      }
> +    }
>  
> -    // Sort the operands by rank.
> -    if (RHSRank < LHSRank) {
> -      I->setOperand(0, RHS);
> -      I->setOperand(1, LHS);
> +    // Transform:
> +    //   FAdd(x, FMul(-0.1234, y)) -> FSub(x, FMul(0.1234, y))
> +    // where the FMul can also be an FDiv, and FAdd can be a FSub.
> +    if (I->getOpcode() == Instruction::FMul ||
> +        I->getOpcode() == Instruction::FDiv) {
> +      if (ConstantFP *LHSConst = dyn_cast<ConstantFP>(I->getOperand(0))) {
> +        if (LHSConst->isNegative() && I->hasOneUse()) {
> +          Instruction *Parent = I->user_back();
> +          if (Parent->getOpcode() == Instruction::FAdd) {
> +            if (Parent->getOperand(0) == I)
> +              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);
> +        }
> +      }
>      }

After fixing the logic for FDiv, the nesting is out of control.  Can you
reorganize?  I suggest something like the following:

    /// Doxygen comment about commuting?
    static bool tryToCommuteFloats(Instruction *I) {
      if (I->getOpcode() != Instruction::FMul &&
          I->getOpcode() != Instruction::FAdd)
        return false;
      // ...
    }

    /// Doxygen comment about your change?
    static bool tryToOptimizeFAddNegExpr(Instruction *I) {
      if (I->getOpcode() != Instruction::FMul &&
          I->getOpcode() != Instruction::FDiv)
        return false;
      ConstantFP *LHSConst = dyn_cast<ConstantFP>(I->getOperand(0)))
      if (!LHSConst)
        return false;
      // ...
    }

    void Reassociate::OptimizeInst(Instruction *I) {
      // ...
      if ((I->getType()->isFloatingPointTy() || I->getType()->isVectorTy())) {
        MadeChange |= tryToCommuteFloats(I);
        MadeChange |= tryToOptimizeFAddNegExpr(I);
        return;
      }
      // ...
    }

If you agree, please split it into three patches.

 1. The old code didn't set MadeChange = true; please fix that.  I
    suspect it's not an observable bug, but if you can find a testcase,
    all the better.
 2. Reorganize the old code with no functionality change.
 3. Add your own function and call it.

I didn't think very long about those function names; maybe you'll find
better ones.

>  
>      return;