[Reassociate] Canonicalizing 'x [+-] (-Constant * y)' isn't always a win

Canonicalizing 'x [+-] (-Constant * y)' is not a win if we don't *know*
we will open up CSE opportunities.

If the multiply was 'nsw', then negating 'y' requires us to clear the
'nsw' flag.  If this is actually worth pursuing, it is probably more
appropriate to do so in GVN or EarlyCSE.

This fixes PR23675.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@238397 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/Transforms/Scalar/Reassociate.cpp b/lib/Transforms/Scalar/Reassociate.cpp
index b677523d70328d19d528635e541be02551cc0ef6..937b9cb1ce36eb50f164461d06bea2011fb4aaf4 100644 (file)
--- a/lib/Transforms/Scalar/Reassociate.cpp
+++ b/lib/Transforms/Scalar/Reassociate.cpp
@@ -1966,38 +1966,35 @@ Instruction *Reassociate::canonicalizeNegConstExpr(Instruction *I) {
   if (!I->hasOneUse() || I->getType()->isVectorTy())
     return nullptr;
 
-  // Must be a mul, fmul, or fdiv instruction.
+  // Must be a fmul or fdiv instruction.
   unsigned Opcode = I->getOpcode();
-  if (Opcode != Instruction::Mul && Opcode != Instruction::FMul &&
-      Opcode != Instruction::FDiv)
+  if (Opcode != Instruction::FMul && Opcode != Instruction::FDiv)
     return nullptr;
 
-  // Must have at least one constant operand.
-  Constant *C0 = dyn_cast<Constant>(I->getOperand(0));
-  Constant *C1 = dyn_cast<Constant>(I->getOperand(1));
-  if (!C0 && !C1)
+  auto *C0 = dyn_cast<ConstantFP>(I->getOperand(0));
+  auto *C1 = dyn_cast<ConstantFP>(I->getOperand(1));
+
+  // Both operands are constant, let it get constant folded away.
+  if (C0 && C1)
     return nullptr;
 
-  // Must be a negative ConstantInt or ConstantFP.
-  Constant *C = C0 ? C0 : C1;
-  unsigned ConstIdx = C0 ? 0 : 1;
-  if (auto *CI = dyn_cast<ConstantInt>(C)) {
-    if (!CI->isNegative() || CI->isMinValue(true))
-      return nullptr;
-  } else if (auto *CF = dyn_cast<ConstantFP>(C)) {
-    if (!CF->isNegative())
-      return nullptr;
-  } else
+  ConstantFP *CF = C0 ? C0 : C1;
+
+  // Must have one constant operand.
+  if (!CF)
+    return nullptr;
+
+  // Must be a negative ConstantFP.
+  if (!CF->isNegative())
     return nullptr;
 
   // User must be a binary operator with one or more uses.
   Instruction *User = I->user_back();
-  if (!isa<BinaryOperator>(User) || !User->getNumUses())
+  if (!isa<BinaryOperator>(User) || !User->hasNUsesOrMore(1))
     return nullptr;
 
   unsigned UserOpcode = User->getOpcode();
-  if (UserOpcode != Instruction::Add && UserOpcode != Instruction::FAdd &&
-      UserOpcode != Instruction::Sub && UserOpcode != Instruction::FSub)
+  if (UserOpcode != Instruction::FAdd && UserOpcode != Instruction::FSub)
     return nullptr;
 
   // Subtraction is not commutative. Explicitly, the following transform is
@@ -2006,14 +2003,9 @@ Instruction *Reassociate::canonicalizeNegConstExpr(Instruction *I) {
     return nullptr;
 
   // Change the sign of the constant.
-  if (ConstantInt *CI = dyn_cast<ConstantInt>(C))
-    I->setOperand(ConstIdx, ConstantInt::get(CI->getContext(), -CI->getValue()));
-  else {
-    ConstantFP *CF = cast<ConstantFP>(C);
-    APFloat Val = CF->getValueAPF();
-    Val.changeSign();
-    I->setOperand(ConstIdx, ConstantFP::get(CF->getContext(), Val));
-  }
+  APFloat Val = CF->getValueAPF();
+  Val.changeSign();
+  I->setOperand(C0 ? 0 : 1, ConstantFP::get(CF->getContext(), Val));
 
   // Canonicalize I to RHS to simplify the next bit of logic. E.g.,
   // ((-Const*y) + x) -> (x + (-Const*y)).
@@ -2023,15 +2015,9 @@ Instruction *Reassociate::canonicalizeNegConstExpr(Instruction *I) {
   Value *Op0 = User->getOperand(0);
   Value *Op1 = User->getOperand(1);
   BinaryOperator *NI;
-  switch(UserOpcode) {
+  switch (UserOpcode) {
   default:
     llvm_unreachable("Unexpected Opcode!");
-  case Instruction::Add:
-    NI = BinaryOperator::CreateSub(Op0, Op1);
-    break;
-  case Instruction::Sub:
-    NI = BinaryOperator::CreateAdd(Op0, Op1);
-    break;
   case Instruction::FAdd:
     NI = BinaryOperator::CreateFSub(Op0, Op1);
     NI->setFastMathFlags(cast<FPMathOperator>(User)->getFastMathFlags());

diff --git a/test/Transforms/Reassociate/basictest.ll b/test/Transforms/Reassociate/basictest.ll
index 015d3b0bee9f2bbfe7c5f2cacdcebcd1d8fa5e9e..caaf7726514dee35d4f4957d52dd629a8849f9c7 100644 (file)
--- a/test/Transforms/Reassociate/basictest.ll
+++ b/test/Transforms/Reassociate/basictest.ll
@@ -202,8 +202,8 @@ define i32 @test14(i32 %X1, i32 %X2) {
   ret i32 %D
 
 ; CHECK-LABEL: @test14
-; CHECK-NEXT: sub i32 %X1, %X2
-; CHECK-NEXT: mul i32 %B2, 47
+; CHECK-NEXT: %[[SUB:.*]] = sub i32 %X1, %X2
+; CHECK-NEXT: mul i32 %[[SUB]], 47
 ; CHECK-NEXT: ret i32
 }
 

diff --git a/test/Transforms/Reassociate/canonicalize-neg-const.ll b/test/Transforms/Reassociate/canonicalize-neg-const.ll
index e85a963f6ddac32b54f90029703b324cad33c8de..465460cb53b1b5ec50e4919c8e1845ec52d70dfd 100644 (file)
--- a/test/Transforms/Reassociate/canonicalize-neg-const.ll
+++ b/test/Transforms/Reassociate/canonicalize-neg-const.ll
@@ -49,18 +49,6 @@ define double @test3(double %x, double %y) {
   ret double %mul3
 }
 
-; Canonicalize (x - -1234 * y)
-define i64 @test4(i64 %x, i64 %y) {
-; CHECK-LABEL: @test4
-; CHECK-NEXT: mul i64 %y, 1234
-; CHECK-NEXT: add i64 %mul, %x
-; CHECK-NEXT: ret i64 %sub
-
-  %mul = mul i64 %y, -1234
-  %sub = sub i64 %x, %mul
-  ret i64 %sub
-}
-
 ; Canonicalize (x - -0.1234 * y)
 define double @test5(double %x, double %y) {
 ; CHECK-LABEL: @test5
@@ -156,3 +144,13 @@ define double @test12(double %x, double %y) {
   %add = fadd double %div, %x
   ret double %add
 }
+
+; Don't create an NSW violation
+define i4 @test13(i4 %x) {
+; CHECK-LABEL: @test13
+; CHECK-NEXT: %[[mul:.*]] = mul nsw i4 %x, -2
+; CHECK-NEXT: %[[add:.*]] = add i4 %[[mul]], 3
+  %mul = mul nsw i4 %x, -2
+  %add = add i4 %mul, 3
+  ret i4 %add
+}