[PATCH] Reassociate x + -0.1234 * y into x - 0.1234 * y
Erik Verbruggen
erik.verbruggen at me.com
Mon Apr 21 05:21:19 PDT 2014
Hi Duncan,
Thanks for the feedback! Fixed everything you brought up, and I attached a new patch. 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:
>
>> [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?
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.
>> + 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?
Woops, good catch!
>> + 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
>>
-- Erik.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Reassociate-x-0.1234-y-into-x-0.1234-y.patch
Type: application/octet-stream
Size: 9239 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140421/b4fa1617/attachment.obj>
More information about the llvm-commits
mailing list