[PATCH] D14891: Replace assert with early-out in tryEmitFMulAdd

[David Majnemer via cfe-commits]

majnemer added a subscriber: majnemer.
majnemer added a comment.

Is it possible to have a test which covers this change?


================
Comment at: lib/CodeGen/CGExprScalar.cpp:2568
@@ -2567,1 +2567,3 @@
+  // that does not have any other uses.
   if (llvm::BinaryOperator* LHSBinOp = dyn_cast<llvm::BinaryOperator>(op.LHS)) {
+    if (LHSBinOp->getOpcode() == llvm::Instruction::FMul &&
----------------
I'd use `auto *LHSBinOp` here, the type is obvious from the right hand side.

================
Comment at: lib/CodeGen/CGExprScalar.cpp:2570-2575
@@ -2571,8 +2569,8 @@
+    if (LHSBinOp->getOpcode() == llvm::Instruction::FMul &&
+        LHSBinOp->hasNUses(0))
       return buildFMulAdd(LHSBinOp, op.RHS, CGF, Builder, false, isSub);
-    }
-  } else if (llvm::BinaryOperator* RHSBinOp =
-               dyn_cast<llvm::BinaryOperator>(op.RHS)) {
-    if (RHSBinOp->getOpcode() == llvm::Instruction::FMul) {
-      assert(RHSBinOp->getNumUses() == 0 &&
-             "Operations with multiple uses shouldn't be contracted.");
+  }
+  if (llvm::BinaryOperator* RHSBinOp = dyn_cast<llvm::BinaryOperator>(op.RHS)) {
+    if (RHSBinOp->getOpcode() == llvm::Instruction::FMul &&
+        RHSBinOp->hasNUses(0))
       return buildFMulAdd(RHSBinOp, op.LHS, CGF, Builder, isSub, false);
----------------
I think `use_empty()` is more canonical than `hasNUses(0)` in this situation.

================
Comment at: lib/CodeGen/CGExprScalar.cpp:2573
@@ +2572,3 @@
+  }
+  if (llvm::BinaryOperator* RHSBinOp = dyn_cast<llvm::BinaryOperator>(op.RHS)) {
+    if (RHSBinOp->getOpcode() == llvm::Instruction::FMul &&
----------------
Ditto here.


http://reviews.llvm.org/D14891