[PATCH] D12096: [InstCombineAddSub opportunities]: More opportunities to factorize FAdd/FSub when unsafeAlgebra is present for Inst

[David Majnemer via llvm-commits]

majnemer added a comment.

Out of curiosity, why doesn't the reassociate pass catch your test cases?


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:451
@@ +450,3 @@
+  // TODO: Overly conservative?
+  if (I->getNumUses() != 1)
+    return nullptr;
----------------
Please use `hasOneUse` instead.

================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:466-467
@@ +465,4 @@
+    Value *C = I->getOperand(1);
+    // (a + b) + c -> a + ( b + c ) ?
+    // (a + b) - c -> a + ( b - c ) ?
+    if (IsOp0OpcodeAdd) {
----------------
Elsewhere, you use A, B and C instead of a, b, and c.  Please be consistent.

================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:573-577
@@ +572,7 @@
+    }
+    // A - (B + C) -> (A - C) - B
+    else if (IsOp1OpcodeAdd) {
+      FactorizeOpcode = FinalOpcode = Instruction::FSub;
+    } else {
+      // A - (B - C) -> (A + C) - B
+      FactorizeOpcode = Instruction::FAdd;
----------------
Please be consistent where you have comments.

================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:772
@@ -602,1 +771,3 @@
+  return performFactorizationAssociative(I);
+  // return nullptr;
 }
----------------
This is commented out.


http://reviews.llvm.org/D12096